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 ReSharper settings for coding guidelines. #115

Merged
merged 2 commits into from
Jan 21, 2016

Conversation

luboshl
Copy link
Contributor

@luboshl luboshl commented Jan 20, 2016

Added team-shared solution level settings for ReSharper. These settings meet some of coding guidelines that are adjustable in R#. Inspection severity for these rules is set to Warning.

(@tonysneed - I cannot found your coding guidelines referencing Coding Guidelines for ASP.NET int GitHub. Where is the reference?)

These coding guidelines was applied:

  1. Use _camelCase for private fields.
  2. Avoid this. unless absolutely necessary
  3. Always specify member visiblity, even if it's the default (i.e. private string _foo; not string _foo;)
  4. The var keyword is to be used as much as the compiler will allow.
  5. When using a type that has a C# keyword the keyword is used in favor of the .NET type name.

@tonysneed
Copy link
Collaborator

Thanks for this @luboshl. The only one I'm hesitant to adopt would be no. 4. Would it be possible to adjust this rule to allow for explicit variable types when the type is not apparent? I believe in R# this is called "use var when evident" and appears to be the current default.

rs-var

@tonysneed
Copy link
Collaborator

I updated our C# Coding Guidelines to reflect the usage of var when type is evident.

@azabluda
Copy link
Contributor

Just FYI: https://github.com/DotNetAnalyzers/StyleCopAnalyzers is a very promising project for those who want to enforce code style rules without forcing all developers to have R#. VS2015 users will see warnings/errors straight in the IDE whilst all others may just run a command-line script prior to commit/push. The same CLI command can eventually be activated on a CI server (once TI gets one).

@tonysneed
Copy link
Collaborator

Good find. There is an R# extension for StyleCop. So maybe we can define the rules in StyleCop and then use them from R# via the extension?

@azabluda
Copy link
Contributor

Not sure how well JetBrains integrates with Roslyn... At least that's what I found on their web page
roslyn

@tonysneed
Copy link
Collaborator

That’s funny .. But does it relate to StyleCop? If not, that’s OK — jokes are allowed. :-)

@azabluda
Copy link
Contributor

Here we shouldn't confuse classical StyleCop and its complete remake StyleCopAnalyzers entirely based on Roslyn. The R# plugin you referred is probably only about StyleCop, which I honestly never used. The obvious advantage of StyleCopAnalyzers is that one doesn't have to install anything, because it's just a standard NuGet package for VS2015. One possible disadvantage might be that we probably can't share the config with R# because of JetBrains' attitude to Roslyn.

@tonysneed
Copy link
Collaborator

I see now .. yes, this is much better than the R# settings. Let’s use it.

@azabluda
Copy link
Contributor

Love your optimism, Tony! 😄

Well, it's a bit easier to say than to actually implement it (from the purely organizational viewpoint). If we want to enforce coding rules, then we certainly should immediately make our existing code compliant, which may result in a massive commit affecting lots of codelines. Therefore we must at least wait until all pending PRs are merged.

Another unclear point is VS2013 (and below) support. I would expect SCA doesn't do anything there (at least no harm), but a few tests definitely won't hurt.

Thus I would cautiously rephrase your

Let's use it!

as let's consider this alternative. 😇

@tonysneed
Copy link
Collaborator

Oh yes, better to not break existing code. Maybe something to consider for the new TE Express project for targeting EF7 (now called CORE 1.0)

I'm wondering if there is even a need for an R# settings file, since it seems like the rules mentioned above are already the defaults?

@azabluda
Copy link
Contributor

Question to @luboshl as I'm still not using R#... 😊

@luboshl
Copy link
Contributor Author

luboshl commented Jan 21, 2016

I'm wondering if there is even a need for an R# settings file, since it seems like the rules mentioned above are already the defaults?

Yes, it is. Because if I have local settings conflicting these rules and there are no solution settings, my settings apply. For example name style for local fields. Default is "_camleCalse". I have "camelCase" in my local settings (This computer in Seetings Layers), so it overrides the default. If there is no rule for solution, my rules apply. If there are rules for the solution, they override rules in This computer layer. So rules for the solution should be set, although they are the same as default.

@tonysneed
Copy link
Collaborator

Thanks Lubos, I get it now. :) So is there any problem for non-R# users to have these R# settings?

@luboshl
Copy link
Contributor Author

luboshl commented Jan 21, 2016

@tonysneed, do you realy want to prefer this?

C#FruitFlavor flavor = fruit.GetFlavor(); // this is preferred because result of GetFlavor is not evident

What if you change return value of GetFlavor() to for example IFruitFlavor? If the interface is compatible (that I excpect), I do not have to refactor anyting, if var is used. That doesn't apply if explicit FruitFlavor is used.

Personally I like var much more in the case above than var result = bool or var i = 0 etc. 😄

@luboshl
Copy link
Contributor Author

luboshl commented Jan 21, 2016

Thanks Lubos, I get it now. :) So is there any problem for non-R# users to have these R# settings?

No, non-R# users just ingore the setting file. Only R# uses it.

@luboshl
Copy link
Contributor Author

luboshl commented Jan 21, 2016

R# settings is just help for users with R#. It's not a constraint for them nor for non-R# users.

@tonysneed
Copy link
Collaborator

Can you change the rule for var?

@luboshl
Copy link
Contributor Author

luboshl commented Jan 21, 2016

Yes, I can. But:

  1. Add ReSharper settings for coding guidelines. #115 (comment)
  2. Then instead of
var context = TestsHelper.CreateNorthwindDbContext(CreateNorthwindDbOptions);

foreach (var item in items) // used in ApplyChanges

var trackingState = item.TrackingState; // used in ApplyChanges

whe shoul use

NorthwindDbContext context = TestsHelper.CreateNorthwindDbContext(CreateNorthwindDbOptions);

foreach (ITrackable item in items) // used in ApplyChanges

TrackingState trackingState = item.TrackingState; // used in ApplyChanges

I think that's not what you mean, is it?

@tonysneed
Copy link
Collaborator

Here are the default settings which I think you can incorporate this PR:

<wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation">
    <s:String x:Key="/Default/CodeStyle/CSharpVarKeywordUsage/ForBuiltInTypes/@EntryValue">UseVarWhenEvident</s:String>
    <s:String x:Key="/Default/CodeStyle/CSharpVarKeywordUsage/ForOtherTypes/@EntryValue">UseVarWhenEvident</s:String>
    <s:String x:Key="/Default/CodeStyle/CSharpVarKeywordUsage/ForSimpleTypes/@EntryValue">UseVarWhenEvident</s:String></wpf:ResourceDictionary>

@tonysneed
Copy link
Collaborator

I believe these mean that, when the type is not evident, allow var but do not require it.

@luboshl
Copy link
Contributor Author

luboshl commented Jan 21, 2016

image

image

image

This is the result. Shall I commit that?

@tonysneed
Copy link
Collaborator

tonysneed commented Jan 21, 2016 via email

@luboshl
Copy link
Contributor Author

luboshl commented Jan 21, 2016

Ok, done.

@tonysneed tonysneed merged commit 1cb4779 into TrackableEntities:develop Jan 21, 2016
@tonysneed
Copy link
Collaborator

I tested it and it's looking good!

@luboshl
Copy link
Contributor Author

luboshl commented Jan 21, 2016

👍

@luboshl luboshl deleted the resharper branch February 13, 2016 18:53
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.

3 participants