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

Add StyleCop including some rules. #5

Closed
nils-a opened this issue Sep 6, 2020 · 9 comments · Fixed by #37
Closed

Add StyleCop including some rules. #5

nils-a opened this issue Sep 6, 2020 · 9 comments · Fixed by #37
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@nils-a
Copy link
Contributor

nils-a commented Sep 6, 2020

As mentioned in cake-contrib/Home#11 a dependency to stylecop should be added, including some rules.

@nils-a nils-a added the enhancement New feature or request label Sep 6, 2020
@nils-a nils-a modified the milestone: 0.2.0 Sep 6, 2020
@nils-a
Copy link
Contributor Author

nils-a commented Sep 17, 2020

@gep13 @AdmiringWorm I feel I need a bit of input (and currently only we three are using CakeContrib.Guidelins):

rules-file or editorconfig

  • I't really easy to deploy an additional rules-file using a nupkg and it's "not-so-easy" to deploy an editorconfig (i.e. we have to copy that before build, like the image.
  • Having the rules in the editorconfig is only supported in VS2019 (or rather: "recent IDEs")
  • It feels like rule-files will be deprecated sometime in the future (though I'm not sure about that)

So my idea is this:

  • do not use rules-file (we're in late 2020 - nothing prevents people from using VS2019)
  • always copy the editorconfig next to the csproj (with an opt-out option)

This leaves the project free to have a more general editorconfig on sln-level. Only drawback I see is that one could technically have a limitless amout of rules-files but the number of editorconfigs is limited to the number of folders in a project.

Would that be ok with you?

what rules to set

I am a bit reluctant to impose "my" rules (i.e. formatting and rules like Cake.7zip) as a starter-package to discuss changes.

Is there a more "seasoned" project whose style and rules I copy to start with?

@gep13
Copy link
Member

gep13 commented Sep 17, 2020

I think what you have suggested here sounds like a good approach, I would be happy to see it implemented as such.

In terms of what rules to apply, and which ones to turn off, bottom line, we are never going to please everyone, there is always going to be disagreement 😄

I would suggest starting out small, and only adding the bare minimum of rules that we can all agree on, and from there, look to expand out to include other rules.

Thoughts?

@nils-a
Copy link
Contributor Author

nils-a commented Sep 17, 2020

Looking at "my" rules from Cake.7zip:

There are only three StyleCop rules in there - two of which I would copy:

  • SA1101 as none
    • I don't like to prefix everything with this
  • SA1200 as none
    • I like my usings outside of namespaces

SA1633 (every file needs a header) should depend on the project, so I'd not include it.

The rest from that rules is Microsoft.NetFramework.Analyzers and Microsoft.CodeAnalysis.FxCopAnalyzers.

As for the editorconfig that was exported from VS and only sightly modified (I think) - so I'd use that and see what we get. 😄

@Jericho
Copy link
Member

Jericho commented Sep 17, 2020

Please make sure that existing rules and editorconfig files do not get overwritten.

@gep13
Copy link
Member

gep13 commented Sep 18, 2020

@nils-a sounds like a good starting point.

I always get this wrong, so I have to ask...

Does this:

end_of_line = crlf

cause problems when also developing on a non-WIndows OS? /cc @AdmiringWorm

@nils-a
Copy link
Contributor Author

nils-a commented Sep 18, 2020

Does this:

end_of_line = crlf

cause problems when also developing on a non-WIndows OS? /cc @AdmiringWorm

I wouldn't know. I have not done any real development on non-windows in a really long time.

@AdmiringWorm
Copy link
Member

@nils-a sounds like a good starting point.

I always get this wrong, so I have to ask...

Does this:

end_of_line = crlf

cause problems when also developing on a non-WIndows OS? /cc @AdmiringWorm

It shouldn't really cause any problems, but it will/may cause annoyance to Unix users, especially if the gitattributes file do not reflect the same as the editorconfig file.

TBH, I am a bit against copying a editorconfig file into the repository.
IMO, there should only be a check if one exist (either in the current directory, or in one of the parent directories).
Both stylecop and editorconfig is more down to user preferences than anything else.

@nils-a
Copy link
Contributor Author

nils-a commented Sep 23, 2020

@AdmiringWorm I tend to agree, though I'd love to give "some" guidance. I thought much about this in last days (thats probably why I've been avoiding this issue ;-) ) and I'm also not so sure about copying the .editorconfig any longer.

Thing is: If we copy an editorconfig next to the csproj (after testing if one is already there and such) we could nontheless "override" settings already present in an .editorconfig one level up (say sln-level). So while I can technically avoid overriding a physical file (like @Jericho wanted) it might well be we're "destroying" a carefully maintained set of rules.

So to catch up on @AdmiringWorm last sentence: It is also possible to emit a warning if stylecop is not referenced and/or no .editorconfig is found.

@AdmiringWorm
Copy link
Member

So to catch up on @AdmiringWorm last sentence: It is also possible to emit a warning if stylecop is not referenced and/or no .editorconfig is found.

Personally, that is the approach I would like. Especially considering that I typically have a root editorconfig file in the root of the repository, and only separate editorconfig files for the test projects.

@nils-a nils-a added this to the 0.4.0 milestone Oct 1, 2020
@nils-a nils-a self-assigned this Oct 1, 2020
@nils-a nils-a closed this as completed in #37 Oct 1, 2020
nils-a added a commit that referenced this issue Oct 1, 2020
Added a rules (CCG0005, CCG0006) to check for stylecop
cake-contrib-bot pushed a commit that referenced this issue Oct 1, 2020
Merge pull request #37 from cake-contrib/feature/GH-5

Added a rules (CCG0005, CCG0006) to check for stylecop
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants