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

Lint too aggressive with empty lines before comments #376

Closed
hueniverse opened this issue Jun 21, 2015 · 7 comments
Assignees
Labels
bug
Milestone

Comments

@hueniverse
Copy link
Member

@hueniverse hueniverse commented Jun 21, 2015

See: https://travis-ci.org/hapijs/vision/jobs/67770146

Basically, it is common to comment at the end of a require block as well as default values in an object. Not sure if this is easy to specify of just to drop this style rule.

@hueniverse hueniverse added the bug label Jun 21, 2015
@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Jun 22, 2015

Yeah when I added the rule I also noticed that default values would be a problem, there is an option to not enable this rule on /* */ style comments but that does not fully solve your issue. The at the end of a require block seems weird, why not at the start?
It is hard to have rules and then not enforce them on edge cases unless we make custom rules like the line after function.
Full rule spec here

@hueniverse

This comment has been minimized.

Copy link
Member Author

@hueniverse hueniverse commented Jun 22, 2015

The way vision uses comments is consistent with the overall style, we just need a way to articulate it.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Jun 22, 2015

Ok then we have 3 options

  1. use /* */ (not really wanted ofc)
  2. disable the rule
  3. Make a custom rule that allows the rule with the edge cases (and probably disable the current rule until then)
@hueniverse

This comment has been minimized.

Copy link
Member Author

@hueniverse hueniverse commented Jun 22, 2015

3 otherwise 2.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Jun 22, 2015

Since 3 includes 2 disable it for now (I might do a PR tomorrow for the new eslint version and I will include it then if no one else has already), and @cjihrig since you made the other custom rules, any way you might be able to make this work?

@cjihrig

This comment has been minimized.

Copy link
Contributor

@cjihrig cjihrig commented Jun 22, 2015

I would recommend dropping this rule. From a quick glance it looks like you're going to require too much context to do anything useful. For example, the rule would have to recognize comments that immediately follow variable declarations whose right hand side is a require() call in the global scope. Or recognize comments inside an object literal whose name is internals.defaults. I think you'd likely end up with something very hacky.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Jun 22, 2015

ok, so 2 :P we could set it as a warning but I'm 99% sure that's not wanted

@geek geek added this to the 5.11.0 milestone Jun 22, 2015
@geek geek self-assigned this Jun 22, 2015
@cjihrig cjihrig closed this in #377 Jun 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.