Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply code formatting config to all source files #406

Open
WildWeazel opened this issue Jun 11, 2023 · 3 comments
Open

Apply code formatting config to all source files #406

WildWeazel opened this issue Jun 11, 2023 · 3 comments
Labels
question Further information is requested refactor Refactor something to make it better

Comments

@WildWeazel
Copy link
Member

#163 introduced a .editorconfig with code formatting rules. Reformat all .cs (and .gd?) files at once. Going forward devs should use this file in their IDE if possible, and format changes accordingly. But first let's settle on what the formatting rules should be.

I recommend we follow Godot's own C# style guide, and Microsoft's as a fallback. Discuss formatting here.

@WildWeazel WildWeazel added question Further information is requested refactor Refactor something to make it better labels Jun 11, 2023
@pcen
Copy link
Contributor

pcen commented Jun 11, 2023

#143 falls under this issue

@pcen
Copy link
Contributor

pcen commented Sep 28, 2023

I need to revisit the .editorconfig, recently my vscode C# got updated (I assume) because it's giving new suggestions that aren't in the editor config and were not previously defaults:

remove "this" qualifier on member variables/methods/properties when they are not shadowing a local non-member identifier

personally, I don't like this default, I prefer members to be qualified with "this" because it makes code easier to read at the expense of being slightly more verbose to type

use explicit type instead of var

I believe this has been our recommended coding style, but I don't think my editor used to complain

use lowercase identifier for builtin types instead of capitalised (ie. double instead of Double)

new expression can be simplified

instead of Label foo = new Label(); this one recommends Label foo = new(); Personally I like this one, since we're opting to not use var on the lhs, I think this is a nice tradeoff of making code less verbose without, imo, reducing clarity

object initialisation can be simplified

here's a real world example from our codebase:
before:

TerrainType c7Terrain = new TerrainType();
c7Terrain.Key = civTerrainKeyLookup[civ3Index];
c7Terrain.DisplayName = civ3Terrain.Name;
c7Terrain.baseFoodProduction = civ3Terrain.Food;
c7Terrain.baseShieldProduction = civ3Terrain.Shields;
c7Terrain.baseCommerceProduction = civ3Terrain.Commerce;
c7Terrain.movementCost = civ3Terrain.MovementCost;
c7Terrain.allowCities = civ3Terrain.AllowCities != 0;
c7Terrain.defenseBonus = new StrengthBonus {
      description = civ3Terrain.Name,
      amount = civ3Terrain.DefenseBonus / 100.0
};

after:

TerrainType c7Terrain = new TerrainType {
  Key = civTerrainKeyLookup[civ3Index],
  DisplayName = civ3Terrain.Name,
  baseFoodProduction = civ3Terrain.Food,
  baseShieldProduction = civ3Terrain.Shields,
  baseCommerceProduction = civ3Terrain.Commerce,
  movementCost = civ3Terrain.MovementCost,
  allowCities = civ3Terrain.AllowCities != 0,
  defenseBonus = new StrengthBonus {
	  description = civ3Terrain.Name,
	  amount = civ3Terrain.DefenseBonus / 100.0
  }
};

The indentation is messed up here but the idea is to assign fields values inside a member initialiser list that's tacked on to the end of the constructor (in this case, it's the default ctor, but this syntax also works for something like new Foo(buz){Bar = bar};

@WildWeazel
Copy link
Member Author

I definitely agree with using 'this' by default. It also helps with autocomplete when typing. Using 'new()' instead of 'var' seems fair.

Object initialization seems cool but I haven't used it enough to have much of an opinion. It does make it easier to hide setters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested refactor Refactor something to make it better
Projects
None yet
Development

No branches or pull requests

2 participants