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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

New: Rule - 'lines-between-class-methods' (fixes #5949) #5950

Closed
wants to merge 1 commit into from

Conversation

LinusU
Copy link
Contributor

@LinusU LinusU commented Apr 24, 2016

This is the first time I'm writing a rule for eslint so please take an extra hard look so that I don't screw anything up :)

Things to do:

  • Bikeshed name
  • Handle comments

Edit:
Handled comments, changed the name and squashed everything into one commit.

Should be ready for review now 馃檶

If anyone is interested in the separate commits, here they are: https://github.com/LinusU/eslint/tree/class-padding-history

@eslintbot
Copy link

LGTM

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @nzakas, @scriptdaemon and @alberto to be potential reviewers

@jquerybot
Copy link

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

馃摑 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.


If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

@LinusU LinusU changed the title [WIP] New: Rule - 'class-padding' (fixes #5949) New: Rule - 'lines-between-class-methods' (fixes #5949) Apr 24, 2016
@nzakas
Copy link
Member

nzakas commented Apr 24, 2016

Marking as "do not merge" because the issue isn't accepted yet

@nzakas nzakas added the do not merge This pull request should not be merged yet label Apr 24, 2016
@LinusU
Copy link
Contributor Author

LinusU commented Jun 18, 2016

@nzakas The issue has been updated to "Accepted" now. Is there anything that I should do at this point?

@ilyavolodin ilyavolodin removed the do not merge This pull request should not be merged yet label Jun 18, 2016
@ilyavolodin
Copy link
Member

Issue is now accepted. Removing "do not merge". @LinusU no, you don't need to do anything yet. We'll code review this and leave you feedback if anything needs to be updated. (It's going to be a little while before we get to this, due to the fact that we are focused on fixing issues with the last release at the moment).

@LinusU
Copy link
Contributor Author

LinusU commented Jun 18, 2016

No stress, thank you for the fast response :)

@ilyavolodin
Copy link
Member

LGTM. You might want to make this rule fixable. Should be pretty easy, since it's just whitespace fixing. Although you might run into trouble with comments.

@nzakas nzakas added the do not merge This pull request should not be merged yet label Jun 20, 2016
@nzakas
Copy link
Member

nzakas commented Jun 20, 2016

There's an open discussion about whether this should be folded into a more general rule about lines after block statements: #3092

@eslintbot
Copy link

LGTM

@LinusU
Copy link
Contributor Author

LinusU commented Aug 3, 2016

Rebased on master, any updates on this?

@vitorbal
Copy link
Member

vitorbal commented Aug 5, 2016

Hi @LinusU, could you make sure the rule adheres to the new rule format that was introduced since you first submitted the PR? More information about the new format can be found here:
http://eslint.org/docs/developer-guide/working-with-rules

Also, we recently switched the no-var rule on for our codebase so it looks like you got a couple of linting errors because of that as well.

@vitorbal
Copy link
Member

vitorbal commented Aug 5, 2016

@LinusU Just to make sure it's still clear: as @nzakas mentioned, until the discussions in #3092 finish, we can't merge this yet.
Thank you for your understanding!

@kaicataldo
Copy link
Member

We've actually consolidated the original issue and a few others into one issue (all dealing with padding blocks with newlines) here: #7356

Really appreciate all the work you did on this rule - we're still trying to decide exactly how we want to proceed. Feel free to join in on the discussion.

@kaicataldo
Copy link
Member

On a side note - it doesn't look like we're going to be going with a separate rule implementation (though nothing is set in stone yet) specifically for lines between class methods, so chances are we won't be able to merge the PR in its current state.

@kaicataldo
Copy link
Member

Ah, looks like there's another PR for this same issue: #6445

@nzakas nzakas removed the CLA: Valid label Oct 31, 2016
@kaicataldo kaicataldo added rule Relates to ESLint's core rules feature This change adds a new feature to ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Nov 6, 2016
@alberto
Copy link
Member

alberto commented Feb 16, 2017

Closing this as we decided to go with a different rule proposal: #7356 (comment)

We appreciate your work here @LinusU and we hope you can keep contributing to eslint. Things don't usually take this long to get sorted out.

@alberto alberto closed this Feb 16, 2017
@kaicataldo
Copy link
Member

Thanks @LinusU for your contribution!

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion do not merge This pull request should not be merged yet evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants