First approach of rule SA1140 MaximumLineLength #2302

pkess · 2017-02-23T13:03:22Z

Hi,
i just created a rule to meet #1808.
The rule will not raise any warnings or errors unless a maximum line length is configured in stylecop.json

Please leave comments

codecov · 2017-02-23T13:11:34Z

Codecov Report

Merging #2302 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2302      +/-   ##
==========================================
+ Coverage   96.84%   96.84%   +<.01%     
==========================================
  Files         598      600       +2     
  Lines       84061    84159      +98     
  Branches     3200     3210      +10     
==========================================
+ Hits        81406    81504      +98     
  Misses       1790     1790              
  Partials      865      865

Impacted Files	Coverage Δ
...Analyzers.Test/ReadabilityRules/SA1140UnitTests.cs	`100% <100%> (ø)`
...lyzers/ReadabilityRules/SA1140MaximumLineLength.cs	`100% <100%> (ø)`
...lyzers/Settings/ObjectModel/ReadabilitySettings.cs	`100% <100%> (ø)`	⬆️
...alyzers.Test/DocumentationRules/SA1623UnitTests.cs	`100% <0%> (ø)`	⬆️
...alyzers.Test/DocumentationRules/SA1612UnitTests.cs	`100% <0%> (ø)`	⬆️
...Analyzers.Test/ReadabilityRules/SA1125UnitTests.cs	`100% <0%> (ø)`	⬆️
...eCop.Analyzers.Test/LayoutRules/SA1513UnitTests.cs	`100% <0%> (ø)`	⬆️
...alyzers.Test/DocumentationRules/SA1624UnitTests.cs	`100% <0%> (ø)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08e960e...871be1b. Read the comment docs.

stylecop.settings is stylecop.json

pkess · 2017-02-23T15:28:30Z

Corrected some copy paste errors and added example for documentation

Tabs are not handled with SA1140

pkess · 2017-03-02T16:46:44Z

Any news here?

vweijsters · 2017-03-07T13:55:35Z

I'll have a look this week. Marked it 'do not merge' to signify that the discussion in #1808 has not come to a conclusion yet.

vweijsters · 2017-03-15T21:27:40Z

StyleCop.Analyzers/StyleCop.Analyzers.Test/ReadabilityRules/SA1140UnitTests.cs

+
+            this.SetLineLengthSettings(41);
+            await this.VerifyCSharpDiagnosticAsync(testCode, new[] { diagnosticLine4 }, CancellationToken.None).ConfigureAwait(false);
+        }


This is missing a test case where all lines are shorter than the maximum line length

vweijsters · 2017-03-15T21:31:33Z

StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1140MaximumLineLength.cs

+    /// than allowed in stylecop.json</para>
+    /// </remarks>
+    [DiagnosticAnalyzer(LanguageNames.CSharp)]
+    internal class SA1140MaximumLineLength : DiagnosticAnalyzer


This should be marked with [NoCodeFix]

vweijsters · 2017-03-15T21:37:13Z

StyleCop.Analyzers/StyleCop.Analyzers/Settings/ObjectModel/ReadabilitySettings.cs

        }
+
+        public int LineLength =>
+            this.lineLength;
    }


LineLength should be named MaximumLineLength to provide more clarity to someone reading the stylecop.json file.

vweijsters · 2017-03-15T21:38:23Z

documentation/ReadabilityRules.md

@@ -39,3 +39,4 @@ Identifier | Name | Description
 [SA1134](SA1134.md) | AttributesMustNotShareLine | An attribute is placed on the same line of code as another attribute or element. 
 [SA1136](SA1136.md) | EnumValuesShouldBeOnSeparateLines | Multiple enum values are placed on the same line of code. 
 [SA1139](SA1139.md) | UseLiteralsSuffixNotationInsteadOfCasting | Use literal suffix notation instead of casting. 
+[SA1140](SA1140.md) | SA1140MaximumLineLength | Code should be wrapped to a maximum line length. 


SA1140MaximumLineLength should be MaximumLineLength, to be inline with the other entries

vweijsters · 2017-03-15T21:39:52Z

documentation/SA1140.md

+</table>
+
+:memo: This rule is new for StyleCop Analyzers, and was not present in StyleCop Classic.
+


Please add a remark that this was added in 1.1.0

vweijsters · 2017-03-15T21:40:55Z

StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1140MaximumLineLength.cs

+        private static readonly string HelpLink = "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1140.md";
+
+        private static readonly DiagnosticDescriptor Descriptor =
+            new DiagnosticDescriptor(DiagnosticId, Title, MessageFormat, AnalyzerCategory.ReadabilityRules, DiagnosticSeverity.Warning, AnalyzerConstants.EnabledByDefault, Description, HelpLink);


This should have AnalyzerConstants.DisabledByDefault

Am i right that the AnalyzerConstants.DisabledByDefault will set the Severity Level to None by default?
I would like this rule to check the line length as soon as a user defined a maximum in his config. Thats the way it works at the moment.

Renamed `SA1140MaximumLineLength` should be `MaximumLineLength`, to be inline with the other entries

Rule was added in 1.1.0

pkess · 2017-03-19T12:43:16Z

Ok, i think finally i implemented all of your requests exept the DisabledByDefault constant.
I am sorry for all those commits but i did not had visual studio installed on the pc i used to implement all those changes.

Please give feedback

pkess · 2017-03-30T17:51:29Z

Hey guys,

Did you already checkout my last changes?

cristobalito · 2017-06-18T22:06:37Z

apologies for unsolicited commenting on this PR - I've nothing to do with project but was looking for this sort of functionality as a user

@pkess - nice work. It would be great to see this merged into the project. While i like your approach w.r.t. to the outstanding issue on this PR, esp. for its convenience, it strikes me as a non-standard way to configure the rule. As a user, I would expect that the value specified in my ruleset ultimately governs whether or not the rule is on. One major benefit of this, is that there is only one obvious place I have to go in order to disable the rule, should I so desire.

In order to break the impasse here, would you be willing to compromise here to get the majority of the benefit into the codebase?

pkess · 2017-06-19T17:23:00Z

Hi,

Thank you for your comment.
In fact I think the way of activating the rule by specification of a line length is very clear. There is another way to specific set the severity level of rules. I don't like to implement a separate way of activation.

Additionally I don't have a machine with Windows and visual studio running now (and will not have any in the next month's) so I am not able to fix it quite now. Feel free to fork my branch and implement it as you like. I would be excited!

sharwell · 2017-06-19T17:53:46Z

@pkess I'm won't hide that I'm not a fan of this rule (unrelated to your implementation of it), but after I get some of the other blocking issues (e.g. C# 7) taken care of I will take a serious look at this PR and see if we can get it included as a disabled-by-default rule.

LordBenjamin · 2017-09-11T09:42:43Z

@sharwell - is there any timeline for including this functionality? It would be useful to know before we decide whether to reference another third-party analyzer or write our own equivalent.

sharwell · 2017-09-11T12:59:26Z

@LordBenjamin Not yet. Feel free to keep reminding me.

cristobalito · 2017-09-11T13:32:45Z

@sharwell If this rule wasn't enabled by default (as per original PR), would you be happy to merge it? If so, I'd be happy to take a look at doing that tonight.

sharwell · 2017-09-11T13:41:07Z

@cristobalito I haven't had a chance to do the code review yet. If the only issues are the merge conflicts and changing it to disabled by default, then those are items we can quickly clean up when we get ready to merge. Thank you though. 😄

promontis · 2017-11-01T13:07:54Z

@sharwell is it possible to merge this?

PeterRockstars · 2018-07-04T11:05:53Z

Any progress on this? Would love to see this rule implemented and it seems that we are on the home stretch. Anyone care to take us the last few yards?

pdelvo

I have a few minor comments on this. This is still pending @sharwell's take on this. All the comments I made could also be addressed in a later PR.

pdelvo · 2018-07-11T19:51:14Z

StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/ReadabilityResources.resx

+    <value>Code should be wrapped to a maximum line length. See stylecop.json</value>
+  </data>
+  <data name="SA1140MessageFormat" xml:space="preserve">
+    <value>Maximum line length exceeded</value>


I think it would be good to change this to "Maximum line length of {0} exceeded" and make it include the set line length.

pdelvo · 2018-07-11T19:56:51Z

StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1140MaximumLineLength.cs

+                {
+                    if ((line.End - line.Start) > readabilityRules.MaximumLineLength)
+                    {
+                        var location = root.SyntaxTree.GetLocation(line.Span);


Maybe only report the diagnostic on the bit that is too long so the user knows how much the line is too long.

pdelvo · 2018-07-11T19:58:26Z

documentation/SA1140.md

+
+## Known issues
+
+Tabs in source file are not expanded to spaces for evaluation of the line length. Only use this rule if spaces are used for indention.


Should we do something about this? We could count the number of tabs in the line and use the configured tab width for that

At the time this was first implemented, tab width might not have been included in stylecop.json. It's now available there so the feature could be implemented in this rule too.

pdelvo · 2018-07-11T19:59:44Z

StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1140MaximumLineLength.cs

+    using StyleCop.Analyzers.Helpers;
+
+    /// <summary>
+    /// Maximum line length


All(?) other rules contain the documentation as part of the xml documentation

danpetitt · 2019-04-29T22:18:32Z

Any news on this? My new fellow devs (company merged) prefer to operate a max line length style and I want something to pre-emptively warn me

dave-q · 2019-05-03T16:08:54Z

Would be very happy to see this as a feature. Any news on progress?

admsugar · 2020-06-17T14:21:00Z

Any update on this?

jnm2 · 2020-06-17T17:27:19Z

I believe all updates will be seen in this thread or the proposal issue, if that's what you're asking.

jhoy1020 · 2024-01-10T23:04:29Z

Is this change going to be checked in? Trying to get my team to standardize on a max line length is impossible without some kind of preventative check. Per documentation, it is recommended to have a max line length of 120 https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/coding-conventions. It would be really helpful to have this change to help enforce this rule.

First approach of rule SA1140 MaximumLineLength

04d19e7

Peter Kessen added 2 commits February 23, 2017 16:22

corrected ref to configuration file and added example

6bbf007

stylecop.settings is stylecop.json

Corrected documentation

edb5ee2

Peter Kessen and others added 3 commits February 24, 2017 08:49

Added tests for SA1140

04caa24

Added known issue to rule description SA1140

4428ef2

Tabs are not handled with SA1140

Merge branch 'master' into MaximumLineLength

e5a5813

vweijsters added code review do not merge labels Mar 7, 2017

reasra mentioned this pull request Mar 8, 2017

Rule proposal: Line Length #1808

Open

vweijsters requested changes Mar 15, 2017

View reviewed changes

pkess added 9 commits March 19, 2017 12:42

Update ReadabilityRules

654050d

Renamed `SA1140MaximumLineLength` should be `MaximumLineLength`, to be inline with the other entries

Added remark

88c7df0

Rule was added in 1.1.0

Added NoCodeFix attribute

7a88422

Added testcase where all lines are ok

5840560

Renamed lineLength in config to maximumLineLength

a73e413

Added reason for NoCodeFix

69e7cc6

Renamed missing LineLength to MaximumLineLength

9f7217d

Corrected test for check with no diagnostic warning

0a9ba63

Resolved Stylcop error in source

871be1b

pdelvo approved these changes Jul 11, 2018

View reviewed changes

bpkroth mentioned this pull request Sep 30, 2020

Fixup comment word wrap settings for vscode vim microsoft/MLOS#122

Merged

		</table>

		:memo: This rule is new for StyleCop Analyzers, and was not present in StyleCop Classic.


		## Known issues

		Tabs in source file are not expanded to spaces for evaluation of the line length. Only use this rule if spaces are used for indention.

First approach of rule SA1140 MaximumLineLength #2302

Are you sure you want to change the base?

First approach of rule SA1140 MaximumLineLength #2302

Conversation

pkess commented Feb 23, 2017

codecov bot commented Feb 23, 2017 • edited Loading

Codecov Report

pkess commented Feb 23, 2017

pkess commented Mar 2, 2017

vweijsters commented Mar 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkess commented Mar 19, 2017

pkess commented Mar 30, 2017

cristobalito commented Jun 18, 2017

pkess commented Jun 19, 2017

sharwell commented Jun 19, 2017 • edited Loading

LordBenjamin commented Sep 11, 2017

sharwell commented Sep 11, 2017

cristobalito commented Sep 11, 2017

sharwell commented Sep 11, 2017

promontis commented Nov 1, 2017

PeterRockstars commented Jul 4, 2018

pdelvo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpetitt commented Apr 29, 2019

dave-q commented May 3, 2019

admsugar commented Jun 17, 2020

jnm2 commented Jun 17, 2020

jhoy1020 commented Jan 10, 2024

codecov bot commented Feb 23, 2017 •

edited

Loading

sharwell commented Jun 19, 2017 •

edited

Loading