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

SA1636, SA1641 place too heavy a burden on a developer #1366

Closed
penartur opened this issue Sep 2, 2015 · 2 comments · Fixed by #1414
Closed

SA1636, SA1641 place too heavy a burden on a developer #1366

penartur opened this issue Sep 2, 2015 · 2 comments · Fixed by #1414
Assignees
Milestone

Comments

@penartur
Copy link

penartur commented Sep 2, 2015

  1. It seems that legacy StyleCop does not enforce SA1636 and SA1641 unless there are stylecop settings. StyleCopAnalyzers enforces it always (except if user explicitly suppresses these on every of their .cs files, or if user explicitly mentions all stylecop rules except SA1636 and SA1641 in their custom ruleset).
  2. It seems that there is no way to comply with SA1636 and SA1641 unless there is stylecop.json (otherwise the comparison will always fail), so the user migrating from old VS versions is forced not only to normalize their file headers, but to create stylecop.json as well.
  3. stylecop.json is ignored by default VS gitignore on github, so the user, if they use github-supplied gitignore, is forced to modify their .gitignore as well (by adding an explicit !stylecop.json line in the end), and to maintain it during github-supplied gitignore updates. What's more, it is not entirely obvious that stylecop.json will be ignored, so, on first attempt, user will commit changes in their project while leaving stylecop.json behind, which means that the project will build successfully with VS 2015 on the said user PC, with VS 2013 on other users PCs, but not with VS 2015 on other users PCs!

As I see it, it is not user-friendly and backwards-compatible at all. Or do I miss something?

Maybe SA1636 and SA1641 should not be checked if there is no stylecop.json present (as in legacy StyleCop)?

@vweijsters
Copy link
Contributor

@penartur thanks for providing feedback

Disabling SA1636 and SA1641 when there is no settings file sounds like a good idea to me. I verified that it is consistent with the legacy StyleCop behavior.

I would also suggest that we rename our settings file to stylecopanalyzers.json to prevent issues with default GitHub ignore policies. Even if we manage to get VisualStudio.gitignore modified, there will still be lots of projects that have the unmodified one and placing the burden on these projects to change .gitignore does not seem like a good plan to me.

@sharwell If there is agreement with the proposed changes, assign it to me please.

@sharwell
Copy link
Member

sharwell commented Sep 2, 2015

I'd prefer to change SA1633 to DisabledByDefault. If all the rules are enabled, users will expect them to find violations.

The VisualStudio.gitignore file is a disaster (meaning no one should ever use it because it has so many bad rules). I'm fond of the original filename; what about a warning when we install the NuGet package or an analyzer to report a misconfigured .gitignore?

Corrections to the original post:

or if user explicitly mentions all stylecop rules except SA1636 and SA1641 in their custom ruleset

You only need to list rules that you change the behavior of. In this case, you could use a ruleset that has only SA1633 listed, and all other rules will use their default behavior.

by adding an explicit !stylecop.json line in the end

Actually, the correct way to restore proper behavior would be removing the line you linked to.

sharwell added a commit to sharwell/StyleCopAnalyzers that referenced this issue Sep 7, 2015
@sharwell sharwell added this to the 1.0.0 Beta 10 milestone Sep 7, 2015
@sharwell sharwell self-assigned this Sep 7, 2015
sharwell added a commit to sharwell/StyleCopAnalyzers that referenced this issue Sep 8, 2015
sharwell added a commit to sharwell/StyleCopAnalyzers that referenced this issue Sep 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants