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 max-lines #3808

Open
pburkindine opened this issue Nov 20, 2018 · 15 comments
Open

Add max-lines #3808

pburkindine opened this issue Nov 20, 2018 · 15 comments
Labels
status: ask to implement ask before implementing as may no longer be relevant type: new rule an entirely new rule

Comments

@pburkindine
Copy link

pburkindine commented Nov 20, 2018

What is the problem you're trying to solve?

Developers create very large S/CSS files which cannot be maintained

What solution would you like to see?

Rule to set an error when the length of a file exceeds a given integer, same concept as tslint:max-file-line-count

@jeddy3
Copy link
Member

jeddy3 commented Nov 21, 2018

@pburkindine Thanks for the suggestion.

There is also a similar rule in ESLint.

I think we'd want to call the rule max-lines. It'll then align with our naming conventions and fit well with the likes of max-empty-lines.

  • Name: max-lines
  • Primary option: int
  • Secondary options: none
  • Message: Expected no more than ${max} ${max === 1 ? "line" : "lines"}
  • Description: "Limit the number of lines."
  • Section: "Limit language features" -> "General / Sheet"

Feel free to contribute this rule.

@jeddy3 jeddy3 changed the title max-file-line-count Add max-lines Nov 21, 2018
@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone type: new rule an entirely new rule good first issue is good for newcomers labels Nov 21, 2018
@Maxim-Mazurok
Copy link
Member

I'm going to work on this one, if nobody else is working already.

@Maxim-Mazurok Maxim-Mazurok added status: wip is being worked on by someone and removed good first issue is good for newcomers status: ready to implement is ready to be worked on by someone labels Nov 23, 2018
@Maxim-Mazurok
Copy link
Member

I'm thinking about adding ignore: ["comments", "empty-lines"] to this new rule. But those options are not requested as of now, but they present in eslint max-lines rule. So, @jeddy3, should I include those options, or wait for the request?

@Maxim-Mazurok Maxim-Mazurok self-assigned this Nov 25, 2018
@jeddy3
Copy link
Member

jeddy3 commented Nov 26, 2018

So, @jeddy3, should I include those options, or wait for the request?

As a rule of thumb, we generally wait until an option is requested before adding it. So, let's just add the plain rule for now.

@cahamilton
Copy link
Member

Hey there. I hope you don't mind me hijacking this issue - it's a rule I'd like to start enforcing myself in my projects.

I've pieced together a really basic implementation in #4021, which should cover the basic requirement for enforcing line count (no secondary options). Let me know if theres any issues with it!

@Maxim-Mazurok
Copy link
Member

@cahamilton, sure, I don't mind at all. I was trying to implement this, but haven't found an optimal way to do this while following general approach used in the code base for other rules.
And then, I switched to other project, without stylelint, so I'm not sure if I'm going to finish this at all.
I will take a look at your PR, thanks!

@cahamilton
Copy link
Member

cahamilton commented Apr 12, 2019

@evilebottnawi @hudochenkov @jeddy3 @Maxim-Mazurok

Working through the PR changes now and was wondering, how do we feel about updating the error message to something like the following?

Expected ${max} ${max === 1 ? "line" : "lines"} max. Actual is ${actual} lines (or something similar).

Would help give more accurate test assertions considering things like different syntaxes, mixed line endings etc.

Would also be somewhat more inline with error messaging found in ESLint, which uses:
File must be at most {{max}} lines long. It's {{actual}} lines long.

@alexander-akait
Copy link
Member

Yep, message from eslint looks better

@jeddy3
Copy link
Member

jeddy3 commented Apr 12, 2019

Would help give more accurate test assertions considering things like different syntaxes, mixed line endings etc.

That makes sense. How's about we standardise on "Found ..." as it's shorter and a single word e.g.

Expected no more than ${max} ${max === 1 ? "line" : "lines"}. Found ${found} ${found === 1 ? "line" : "lines"}.

The "Expected ..." convention is shared across all stylelint rules. And the "no more than" is shared across the *-max-* rules, so we should stick with them for this rule.

@pburkindine
Copy link
Author

@jeddy3 @Maxim-Mazurok @cahamilton any update here?

@alexander-akait
Copy link
Member

PR welcome, it is open source

@Maxim-Mazurok
Copy link
Member

No, @pburkindine sorry, I'm not using stylelint currently. Hopefully, @cahamilton will take care of it in #4021

@jeddy3
Copy link
Member

jeddy3 commented Jan 17, 2022

The community is welcome to create a plugin for this.

@jeddy3 jeddy3 closed this as completed Jan 17, 2022
@alexander-akait
Copy link
Member

@jeddy3 I think this rule more about potential problems rather than stylistics, preventing many lines can help to maintenance big code base, yep, it sounds like stylistics, but this is not actually an indication of style, it's more about how to write code

@jeddy3 jeddy3 reopened this Jan 17, 2022
@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone and removed status: wip is being worked on by someone labels Jan 17, 2022
Copy link
Contributor

This issue is older than one month. Please ask before opening a pull request, as it may no longer be relevant.

@github-actions github-actions bot added the status: ask to implement ask before implementing as may no longer be relevant label Jan 22, 2024
@github-actions github-actions bot removed the status: ready to implement is ready to be worked on by someone label Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ask to implement ask before implementing as may no longer be relevant type: new rule an entirely new rule
Development

Successfully merging a pull request may close this issue.

5 participants