DotNetAnalyzers / StyleCopAnalyzers

An implementation of StyleCop rules using the .NET Compiler Platform
MIT License
2.66k stars 508 forks source link

Support for nontraditional casing in namespaces (SA1300) #2923

Closed jedidja closed 4 years ago

jedidja commented 5 years ago

Applying StyleCop analyzers to an existing codebase is always difficult, but what happens when the company name begins with a small letter, e.g. eBay? How can you apply SA1300 to everything except the namespaces? Imagine tens of thousands of files with several hundred namespaces like:

eBay.Services.Shipping
eBay.Services.Shipping.Entities
eBay.Services.Auction
eBay.Services.Auction.Bidding

Although normally namespace names are PascalCase, Microsoft admits in their naming guidelines that

If your brand employs nontraditional casing, you should follow the casing defined by your brand, even if it deviates from normal namespace casing.

If someone were to do the work and create a PR, would it be acceptable to either:

a) include a "known terms dictionary" that SA1300 could use to ignore certain words? b) make a very specific exception for namespaces since company names do not always follow PascalCase?

Or, if neither of those would be accepted, is there any way to accomodate this situation?

vweijsters commented 5 years ago

The preferable option for me would be a combination of both listed options: Add a configuration option to ignore certain words for namespaces only. The ignored words will then have to be matched in the exact case that they are specified in, to guard the consistency as much as possible.

jedidja commented 5 years ago

@vweijsters Thanks for the response; your idea makes sense to me as I think it still keeps the underlying spirit of the StyleCop guideline alive while allowing for real-world issues.

I took a look at the contributing guidelines but I'm still unsure if it's worth spending any time on creating a PR for this yet, or if I should wait for feedback from @sharwell or someone else first? It seems like perhaps I would at least wait for a specific label to be applied and then go from there? (Maybe I'm not even the right person to create the PR :))

sharwell commented 5 years ago

@jedida can you add a proposal for the exact change to stylecop.json for this?

jedidja commented 5 years ago

@sharwell sure; here's a first stab at it :) Happy to iterate since I've not done this before...

Property: allowedNamespaceComponentTerms

Default Value: []

Summary: Specifies non-PascalCase terms that would not normally qualify for a namespace component (e.g. company or brand names) but should be allowed. The term only matches a specific component, not a subset. For example, eBay would match eBay.Services.Auction but not eBayServices.Auction.

{
  "settings": {
    "namingRules": {
      "allowedNamespaceComponentTerms": [
        "eBay",
        "iPod",
      ]
    }
  }
}
mzhukovs commented 5 years ago

Totally agree - get this issue with Xamarin projects out the box thanks to iOS and a way to specify exceptions would be ideal.

mzhukovs commented 5 years ago

Thoughts on not limiting to just namespaces for the exceptions? @Jedidja @sharwell

jedidja commented 5 years ago

This is in progress now. @vweijsters after starting this, I think it's sufficient to just have the allowedNamespaceComponentTerms array, which matches nicely with the existing allowedHungarianPrefixes settings. Adding a separate configuration option for enabling the allowed namspaces seems unnecessary.

@mzhukovs I'd rather keep this PR to just namespaces since I've got a clear idea of what I'm trying to accomplish :) I think you could probably follow a similar pattern for exceptions (or possibly build off of this one later).

jedidja commented 5 years ago

PR at https://github.com/DotNetAnalyzers/StyleCopAnalyzers/pull/2996

jedidja commented 5 years ago

Oh, oops .. forgot to ping @sharwell :) I can't seem to add a "code review" tag to the PR, nor add the appropriate label to this issue.

jedidja commented 5 years ago

@sharwell @vweijsters Is there someone who could look at the PR? :) Not sure who else to include on this. Thank you!

sharwell commented 5 years ago

I once had a project with exactly this type of namespace, but no longer believe that was a correct choice. The one thing I still need to consider is whether a better approach here would be to leave this exception to external suppression analyzers (a new feature in Roslyn 3+ which allows a post-processing analyzer to erase false positives).

cytron201 commented 4 years ago

@jedidja @sharwell This is exactly what i've been looking for. Our company has a branding choice which results in a lowercase first letter for an element in the namespaces. A way to allow exclusions in the Stylecop.json is spot on.

Any way of raising this to get it reviewed?

mlessmann commented 4 years ago

This feature is the only thing blocking our company from adopting StyleCop.Analysers. We followed Microsofts guideline and used our branding in our namespaces. The proposed whitelist mechanism would work well for us.

jedidja commented 4 years ago

@sharwell Even if Roslyn 3+ does support post-processing, perhaps this addition is still worthwhile for people working in older environments?

I'm happy to update the PR if anything else is required, but I'm not sure if it'll get approved still :)

sharwell commented 4 years ago

@jedidja I'm planning to merge/release #2996 once a few details are cleared up:

  1. Do we want to configure namespaces or namespace components?
  2. The descriptors 'component' and 'term' are redundant. We should stick to one, and I'm leaning towards 'component'.
jedidja commented 4 years ago

@sharwell Thank you for looking at the PR and the comments; will push an update in the next ten minutes or so.

  1. I was hoping to configure namespace components
  2. Totally agree with component

Update: PR updated

sharwell commented 4 years ago

@jedidja this is now released to NuGet

jedidja commented 4 years ago

@mzhukovs @mlessmann @cytron201 Hope this works out for you :) Thanks @sharwell for merging this!