consider relaxing the "maxlen" option in some situations #1282

Closed
julienw opened this Issue Sep 25, 2013 · 11 comments

Projects

None yet

3 participants

Contributor
julienw commented Sep 25, 2013

We use the "maxlen" option which is generally good.

However there are special cases where we want it to be relaxed. This is mainly in comments, when using URLs.

Could it be possible to relax this rule when the line does not contain any space after the first \w character ?

Owner
valueof commented Oct 8, 2013

You should use locally scoped /*jshint ... */ rules to relax the rule on a case-by-case basis. I think maxlen should be as strict as it is today.

@valueof valueof closed this Oct 8, 2013
Contributor
julienw commented Oct 8, 2013

"locally scoped" for method comments means changing it for a whole file, which does not sound like "case by case" to me.

I think that without this change, maxlen is not very useful in the end.

Owner
valueof commented Oct 8, 2013

You can also set JSHint options per function?

Contributor
julienw commented Oct 8, 2013

What I meant with "method comments" as more jsdoc, I mean a comment that is before the function.

If you can make that function-scope options applies to that function's previous comment aka jsdoc, then this would be good enough for me.

That said, I don't get why the proposed behaviour is undesirable. If time resource is the problem, I'm perfectly willing to make a patch, as this will make it easier to use jshint in Firefox OS's Gaia.

@julienw julienw added a commit to julienw/jshint that referenced this issue Oct 25, 2013
@julienw julienw Issue #1282: relax the "maxlen" option in some situations
This commit relax the "maxlen" option for the following situations:
* the line is a comment AND
* it contains no space except the leading spaces, and the one space between the
  comment opening or a litteral '*' and the text
a28550e
@julienw julienw added a commit to julienw/jshint that referenced this issue Oct 25, 2013
@julienw julienw Issue #1282: relax the "maxlen" option in some situations
This commit relax the "maxlen" option for the following situations:
* the line is a comment AND
* it contains no space except the leading spaces, and the one space between the
  comment opening or a litteral '*' and the text
2468f20
Contributor
julienw commented Oct 25, 2013

please see the PR in #1326

I would really like that jshint do it.

@valueof valueof added a commit that referenced this issue Nov 22, 2013
@julienw @valueof julienw + valueof Fixed #1282: Relax maxlen option in some situations.
Signed-off-by: Anton Kovalyov <anton@kovalyov.net>
03a093a
Contributor
julienw commented Jan 6, 2014

Trying it in the real now, I see this doesn't relax such code:

/*
  long line
*/

ie in a comment, with only some indentation space before the long line. Even one space doesn't work.

I'll do a small PR to relax this a little more.

Contributor
julienw commented Jan 6, 2014

When I have a file containing only a comment block like:

/*
 * long line
 */

then I don't have state.tokens.curr.comment === true and therefore the "shouldTriggerError" variable is true.

Do you know why this happen @antonkovalyov ?

The tests are passing and I can't make it fail even adding or removing new code to the test...

Contributor
julienw commented Jan 6, 2014

More precisely, here is the failing code:

/*
 * http://www.google.fr/ojoiuroioiurotiurtuortorutouorutoruotuotuorutorutorutoiuroituoriutoirut
 */

// http://www.google.fr/ojoiuroioiurotiurtuortorutouorutoruotuotuorutorutorutoiuroituoriutoirut
/* http://www.google.fr/ojoiuroioiurotiurtuortorutouorutoruotuotuorutorutorutoiuroituoriutoirut
 */

The first line long line is triggering the error, but not the 2 other lines. This happens because state.tokens.curr.comment is false.

However, the test in 03a093a#diff-cf5c42a406c521307a32555910336a6bR10 does not show the error, so I don't understand exactly what's happening. I can try to fix this if you help me finding the issue...

Contributor
julienw commented Jan 8, 2014

found the issue, PR is coming

@julienw julienw added a commit to julienw/jshint that referenced this issue Jan 8, 2014
@julienw julienw issue #1282: consider relaxing the "maxlen" option in comments for so…
…me cases

Correctly fix the issue for multiline comments
2b173b0
Contributor
julienw commented Jan 8, 2014

Also, forget my comment #1282 (comment), the issue was finally something else.

@valueof valueof added a commit that referenced this issue Jan 21, 2014
@julienw @valueof julienw + valueof Issue #1464: Correctly fix #1282 (relaxing maxlen for comments)
Signed-off-by: Anton Kovalyov <anton@kovalyov.net>
c7959ff
@julienw julienw referenced this issue in jscs-dev/node-jscs Jan 22, 2014
Closed

port the "maxlen" relax from jshint issue #1282 #207

threez commented Mar 15, 2014

Thx.

@jugglinmike jugglinmike added a commit to jugglinmike/jshint that referenced this issue Oct 21, 2014
@julienw @jugglinmike julienw + jugglinmike Fixed #1282: Relax maxlen option in some situations.
Signed-off-by: Anton Kovalyov <anton@kovalyov.net>
3956703
@jugglinmike jugglinmike added a commit to jugglinmike/jshint that referenced this issue Oct 21, 2014
@julienw @jugglinmike julienw + jugglinmike Issue #1464: Correctly fix #1282 (relaxing maxlen for comments)
Signed-off-by: Anton Kovalyov <anton@kovalyov.net>
63546fd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment