Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

Conversation

@bamarsha
Copy link
Contributor

@bamarsha bamarsha commented Jul 9, 2020

This is the follow-up to #496, and is part of #342. All C# projects use StyleCop and have been reformatted. I disabled a few more default StyleCop rules; see .editorconfig and src/Common/stylecop.json.

Also, I decided to leave some code in place that violates the default style rules because it requires non-trivial fixes or fixes that are breaking changes (due to renaming publicly accessible things). Normally, I would disable the rules if I didn't want to fix them, but I think the rules are good to have for new code.

Style violations that would have breaking change fixes:

Full disclosure: I did rename some parameters in public methods that started with an uppercase letter (all parameter names should be camelCase), which is technically breaking if you use named parameters, but the risk to renaming parameters seems lower compared to renaming fields.

Other style violations that need non-trivial (but not breaking) fixes:

  • SA1124: Do Not Use Regions. I decided not to remove the regions right now, since removing regions might mean other organization strategies are needed instead.
  • SA1600: Elements Must Be Documented. All public elements should have at least a <summary> or an <inheritdoc/> (for overrides). We have lots of undocumented public things, but I think we should keep the warnings as a reminder to fix them and to write documentation for new code.

@ScottCarda-MS
Copy link
Contributor

It looks like a fair amount of the changes in this PR are the result of applying the new style rules to existing code. Could you please point out the changes that cause the new style rules to be implemented so we can separate out cause from effect in the code changes?

@ScottCarda-MS
Copy link
Contributor

ScottCarda-MS commented Jul 9, 2020

Could you please make tasks for some of the easier to fix violations listed in the description? The SA1600 violations might be too much work for a single task, but the others seem like they could be addressed by someone without taking too much time. The ones that are breaking changes would need to be discussed and documented properly before changing them, but I think we should at least create the tasks to get the ball rolling on that.

@bamarsha
Copy link
Contributor Author

bamarsha commented Jul 9, 2020

It looks like a fair amount of the changes in this PR are the result of applying the new style rules to existing code. Could you please point out the changes that cause the new style rules to be implemented so we can separate out cause from effect in the code changes?

.editorconfig and src/Common/stylecop.json determine the style rules by modifying StyleCop's default rule set. StyleCop has been added to each C# project with two lines:

  • <PackageReference Include="StyleCop.Analyzers" Version="1.2.0-beta.164" PrivateAssets="All" />
  • <AdditionalFiles Include="..\..\Common\stylecop.json" Link="stylecop.json" />

@bamarsha
Copy link
Contributor Author

bamarsha commented Jul 9, 2020

Could you please make tasks for some of the easier to fix violations listed in the description? The SA1600 violations might be too much work for a single task, but the others seem like they could be addressed by someone without taking too much time. The ones that are breaking changes would need to be discussed and documented properly before changing them, but I think we should at least create the tasks to get the ball rolling on that.

My understanding of the coding style guidelines for the team is that we shouldn't fix style issues in existing code unless it's part of other changes to a particular area of the code. Of course, this PR violates that rule, but I think it's an exception because the current coding style can be hard to read and is applied inconsistently, so doing a one-time restyling will help readability and productivity.

For the remaining style violations, my plan was to use the style warnings as TODOs for things to fix if those parts of the code are being changed later, instead of creating separate tasks. However, the extra build warnings in the meantime are not ideal, so I could suppress the warnings for the existing code with #pragma warning disable lines, if that would be better.

@ScottCarda-MS
Copy link
Contributor

Could you please make tasks for some of the easier to fix violations listed in the description? The SA1600 violations might be too much work for a single task, but the others seem like they could be addressed by someone without taking too much time. The ones that are breaking changes would need to be discussed and documented properly before changing them, but I think we should at least create the tasks to get the ball rolling on that.

My understanding of the coding style guidelines for the team is that we shouldn't fix style issues in existing code unless it's part of other changes to a particular area of the code. Of course, this PR violates that rule, but I think it's an exception because the current coding style can be hard to read and is applied inconsistently, so doing a one-time restyling will help readability and productivity.

For the remaining style violations, my plan was to use the style warnings as TODOs for things to fix if those parts of the code are being changed later, instead of creating separate tasks. However, the extra build warnings in the meantime are not ideal, so I could suppress the warnings for the existing code with #pragma warning disable lines, if that would be better.

For the code that is not public facing, I think I might like the warnings as a kind of ToDo list. I would think the guidelines on not fixing style issues wouldn't apply to correcting inconsistent style in public facing API's like the ones that cause the breaking violations. It just seems to be a matter of presenting professionalism in our product to have the API style consistent, and I believe that needs special attention.

@bamarsha
Copy link
Contributor Author

bamarsha commented Jul 10, 2020

I should clarify what the naming issues are:

  1. Protected field SubSelector in SelectByFoldingOverExpressions.StatementTransformation should start with a lowercase letter.
  2. In LanguageServer/Program.cs, the protected fields CONNECTION_VIA_SOCKET and CONNECTION_VIA_PIPE should use PascalCase.
  3. In CommandLineTool/Options.cs and CommandLineTool/Program.cs, there are several protected/public constants that use UPPER_CASE.

The first one is really minor, and I'm also not sure why StyleCop decided that protected fields should start with a lowercase letter.

The last two are only technically breaking changes because they're public - I don't think the public API of the language server and command line compiler is really intended to be used, since they are executable projects. I'm not sure why these fields are public, either - it seems like they could simply be made private/internal.

Copy link
Contributor

@cesarzc cesarzc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @SamarSha for taking the time to do this!

@bamarsha bamarsha merged commit ceede3b into master Jul 14, 2020
@bamarsha bamarsha deleted the samarsha/stylecop2 branch July 14, 2020 02:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants