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

Starting using DocumentationAnalyzers #21095

Merged
merged 1 commit into from
Jun 3, 2020
Merged

Starting using DocumentationAnalyzers #21095

merged 1 commit into from
Jun 3, 2020

Conversation

roji
Copy link
Member

@roji roji commented Jun 1, 2020

Just racking up the consistency points on a fine Monday morning.

Closes #21094

@roji roji requested a review from AndriySvyryd June 1, 2020 11:57
@roji roji requested a review from dougbu as a code owner June 1, 2020 11:57
@roji
Copy link
Member Author

roji commented Jun 1, 2020

/cc @sharwell

@@ -117,7 +117,7 @@ public override void Rollback()
/// Releases any resources used by the transaction and rolls it back.
/// </summary>
/// <param name="disposing">
/// <see langword="talse"/> to release managed and unmanaged resources;
/// <see langword="true"/> to release managed and unmanaged resources;
Copy link
Member Author

Choose a reason for hiding this comment

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

talse!

Copy link
Member

@AndriySvyryd AndriySvyryd Jun 2, 2020

Choose a reason for hiding this comment

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

What starts with "t" and is not false?

Copy link
Member Author

Choose a reason for hiding this comment

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

That must be the 3-value logic thing everyone's been talking about...

EFCore.ruleset Outdated Show resolved Hide resolved
@bricelam
Copy link
Contributor

bricelam commented Jun 1, 2020

Should we dump StyleCop? (We only use it for its documentation rules)

@sharwell
Copy link
Member

sharwell commented Jun 1, 2020

Should we dump StyleCop? (We only use it for its documentation rules)

There isn't a bunch of overlap between the two analyzer sets. Many of the rules in DocumentationAnalyzers were added after they were rejected from StyleCop Analyzers (most often for being too vague or too specific to certain developer scenarios). I'd need to see more details about which rules are used to give a better answer.

@roji
Copy link
Member Author

roji commented Jun 1, 2020

@bricelam aren't there other, non-docs-related rules we could enable from StyleCop? Or is FxCop the one we want (#18870)?

@sharwell
Copy link
Member

sharwell commented Jun 1, 2020

For code style rules, the in-box experience from .editorconfig is intended to provide the most seamless experience. We're currently in the process of making a separate analyzer package for command line enforcement of these rules. Other analyzer packages generally have varying degrees of overlap while often offering unique features as well. Each analyzer incurs a small cost, so the team would need to decide which ones are providing the most overall value.

src/Directory.Build.props Outdated Show resolved Hide resolved
@roji roji merged commit 0065e1d into master Jun 3, 2020
@roji roji deleted the DocConsistency branch June 3, 2020 07:14
@bricelam
Copy link
Contributor

bricelam commented Jun 3, 2020

@bricelam aren't there other, non-docs-related rules we could enable from StyleCop? Or is FxCop the one we want (#18870)?

No other rules--we just use it to check the correctness of XML docs (e.g. no <returns> on a void method).

Smit and (I think) Andriy maintain the .editorconfig file for enforcing our style guidelines. Personally, I'm against enforcing style at all since it limits artistic expression, but on the other hand, I agree that some level of consistency is needed.

I definitely still want to enable more FxCop rules. I'm sure they'll catch some globalization bugs we've missed.

@AndriySvyryd
Copy link
Member

it limits artistic expression

One of the most important aspects in software engineering 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Start using Roslyn documentation analyzers
5 participants