Unity-Technologies / com.unity.selection-groups

Other
117 stars 9 forks source link

Update code styling for consistency #216

Open MechWarrior99 opened 2 years ago

MechWarrior99 commented 2 years ago

Description

Currently the code styling and conventions are quite inconsistent, and many times go against either a Unity convention or C# convention. This makes the code harder to read, feel messy, and harder to contribute to because it is hard to know what convention to follow.

Enums

Currently enums use all caps for their value declarations. This goes against the C# style guidelines, and likewise against how it is done in the rest of Unity. SelectionGroupToolType.LOCK would be changed to SelectionGroupToolType.Lock.

Statics and Consts

Unity has a convention that is different than C# convention. private static fields should be prefixed with s_ and use PascelCase private static Color ProTextColor; would be changed to private static Color s_ProTextColor; And public static fields use camelCase with no prefix.

private constants use the k prefix with PascelCase. private const string AddGroup = ""; would be private const string kAddGroup = ""; and private const int CUR_SG_VERSION = ... would be private const int CurrentSGVersion = ...

Properties

Unity goes against C# guidelines here again and uses camelCase instead of PascelCase for all properties. public int Count => members.Count; would be public int count => members.Count;

Brackets

In Unity, almost every package and the source code always put brackets on their own line

private void RemoveNullMembers() {
...
}

would be

private void RemoveNullMembers() 
{
...
}

I do realize this is a smaller thing, and most methods in SG seem to use the latter style. However there are still many methods that use the former.

Declaration Indentation

Currently some field declarations add indentation so that things are inline, this isn't how the vast majority of the Unity codebase does it. This again is a smaller style thing, but does make it inconsistent and hard to know what convention to follow when looking to make PRs

private const string AddGroup    = "Add Group";
private const int     RightMargin = 16;

would be

private const string AddGroup = "Add Group";
private const int RightMargin = 16;

Declaration Location

The majority of the Unity code base has fields and properties declared at the top of the class, though some parts don't. I am not particularly bothered about this one, however being consistent would be very nice. Right now some classes in SG declare them at the bottom while others at the top.

Access Keywords

Some declarations in SG explicitly state there accessibility (public, internal private, etc.), while others it is implicit. Right now most of the Unity codebase uses explicit, however I believe the Cinemachine guys said that Unity is moving to implicit. Frankly I don't particularly care which, but it would be nice if the SG codebase was consistent in which one it was doing.

Conclusion

These are small things, but they add up and are all over the code of SG, and it makes it hard to read and more so to contribute to. I would be happy to make a PR with the changes if you would like. Just tell me what styling you want to follow.

sindharta commented 2 years ago

Let me get back to you soon on this.

sindharta commented 2 years ago

Thank you for the feedback. After discussing with the team, we have decided to match Unity's convention, especially for public items, moving forward. Specifically:

  1. Enums: use PascalCase, i.e: SelectionGroupToolType.Lock
  2. Properties: use camelCase, i.e: public int groupCount => 0;

For the others, we are fine with how they are at the moment, and you can use your preferred style when creating a pull request.