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 exceptions for __dirname and __filename in dangling underbar check. #165

Closed
wants to merge 1 commit into from
Closed

Add exceptions for __dirname and __filename in dangling underbar check. #165

wants to merge 1 commit into from

Conversation

jacksonrayhamilton
Copy link

Node developers frequently interface with the __dirname and __filename variables. While their globality may be tolerable via the node option, their usage is still considered an error due to the dangling underbar check. You must also enable nomen to skip the check, which is undesirable.

Either one lazily enables nomen for the entire file (permitting developers to define _private methods), or he "temporarily" enables the option...

/*jslint node: true */

/*jslint nomen: true */
var a = __dirname;
/*jslint nomen: false */

...which gets messy.

The workarounds are not good. Furthermore, node developers are stuck with this API. It's not within our power to make better nomenclature decisions.

On the Google Plus community, you have stated that the nomen option will be removed soon. If that were to happen today, it would be impossible for node developers to pass JSLint. That stinks. We just used an API that was given to us; we didn't do anything wrong.

With these points in mind, you should consider adding a special case for tolerating the dangling underbars in the __dirname and __filename variables if the node option is enabled. The variables should be tolerated in their entirety; not just to the extent that they are recognized as poorly-named globals. This will be more in-line with users' expectations, work into the future when nomen goes away, and best of all free developers of silly workarounds to comply with a rule that cannot be complied with.

Tests

master currently returns the following:

> JSLINT('var a = __dirname;', {node: true})
false
> JSLINT.data().errors
[ { id: '(error)',
    raw: 'Unexpected dangling \'_\' in \'{a}\'.',
    code: 'dangling_a',
    evidence: 'var a = __dirname;',
    line: 1,
    character: 9,
    a: '__dirname',
    b: undefined,
    c: undefined,
    d: undefined,
    reason: 'Unexpected dangling \'_\' in \'__dirname\'.' } ]
> JSLINT('var a = __dirname;')
false
> JSLINT.data().errors
[ { id: '(error)',
    raw: 'Unexpected dangling \'_\' in \'{a}\'.',
    code: 'dangling_a',
    evidence: 'var a = __dirname;',
    line: 1,
    character: 9,
    a: '__dirname',
    b: undefined,
    c: undefined,
    d: undefined,
    reason: 'Unexpected dangling \'_\' in \'__dirname\'.' },
  { id: '(error)',
    raw: '\'{a}\' was used before it was defined.',
    code: 'used_before_a',
    evidence: 'var a = __dirname;',
    line: 1,
    character: 9,
    a: '__dirname',
    b: undefined,
    c: undefined,
    d: undefined,
    reason: '\'__dirname\' was used before it was defined.' } ]
> JSLINT('__dirname = 0;', {node: true})
false
> JSLINT.data().errors
[ { id: '(error)',
    raw: 'Unexpected dangling \'_\' in \'{a}\'.',
    code: 'dangling_a',
    evidence: '__dirname = 0;',
    line: 1,
    character: 1,
    a: '__dirname',
    b: undefined,
    c: undefined,
    d: undefined,
    reason: 'Unexpected dangling \'_\' in \'__dirname\'.' },
  { id: '(error)',
    raw: 'Read only.',
    code: 'read_only',
    evidence: '__dirname = 0;',
    line: 1,
    character: 1,
    a: '__dirname',
    b: undefined,
    c: undefined,
    d: undefined,
    reason: 'Read only.' } ]
> JSLINT('__dirname = 0;')
false
> JSLINT.data().errors
[ { id: '(error)',
    raw: 'Unexpected dangling \'_\' in \'{a}\'.',
    code: 'dangling_a',
    evidence: '__dirname = 0;',
    line: 1,
    character: 1,
    a: '__dirname',
    b: undefined,
    c: undefined,
    d: undefined,
    reason: 'Unexpected dangling \'_\' in \'__dirname\'.' },
  { id: '(error)',
    raw: '\'{a}\' was used before it was defined.',
    code: 'used_before_a',
    evidence: '__dirname = 0;',
    line: 1,
    character: 1,
    a: '__dirname',
    b: undefined,
    c: undefined,
    d: undefined,
    reason: '\'__dirname\' was used before it was defined.' } ]
> JSLINT('var __dirname = 0;', {node: true})
false
> JSLINT.data().errors
[ { id: '(error)',
    raw: 'Unexpected dangling \'_\' in \'{a}\'.',
    code: 'dangling_a',
    evidence: 'var __dirname = 0;',
    line: 1,
    character: 5,
    a: '__dirname',
    b: undefined,
    c: undefined,
    d: undefined,
    reason: 'Unexpected dangling \'_\' in \'__dirname\'.' },
  { id: '(error)',
    raw: 'Read only.',
    code: 'read_only',
    evidence: 'var __dirname = 0;',
    line: 1,
    character: 5,
    a: '__dirname',
    b: undefined,
    c: undefined,
    d: undefined,
    reason: 'Read only.' } ]
> JSLINT('var __dirname = 0;')
false
> JSLINT.data().errors
[ { id: '(error)',
    raw: 'Unexpected dangling \'_\' in \'{a}\'.',
    code: 'dangling_a',
    evidence: 'var __dirname = 0;',
    line: 1,
    character: 5,
    a: '__dirname',
    b: undefined,
    c: undefined,
    d: undefined,
    reason: 'Unexpected dangling \'_\' in \'__dirname\'.' } ]

My branch returns the following:

> JSLINT('var a = __dirname;', {node: true})
true
> JSLINT('var a = __dirname;')
false
> JSLINT.data().errors
[ { id: '(error)',
    raw: 'Unexpected dangling \'_\' in \'{a}\'.',
    code: 'dangling_a',
    evidence: 'var a = __dirname;',
    line: 1,
    character: 9,
    a: '__dirname',
    b: undefined,
    c: undefined,
    d: undefined,
    reason: 'Unexpected dangling \'_\' in \'__dirname\'.' },
  { id: '(error)',
    raw: '\'{a}\' was used before it was defined.',
    code: 'used_before_a',
    evidence: 'var a = __dirname;',
    line: 1,
    character: 9,
    a: '__dirname',
    b: undefined,
    c: undefined,
    d: undefined,
    reason: '\'__dirname\' was used before it was defined.' } ]
> JSLINT('__dirname = 0;', {node: true})
false
> JSLINT.data().errors
[ { id: '(error)',
    raw: 'Read only.',
    code: 'read_only',
    evidence: '__dirname = 0;',
    line: 1,
    character: 1,
    a: '__dirname',
    b: undefined,
    c: undefined,
    d: undefined,
    reason: 'Read only.' } ]
> JSLINT('__dirname = 0;')
false
> JSLINT.data().errors
[ { id: '(error)',
    raw: 'Unexpected dangling \'_\' in \'{a}\'.',
    code: 'dangling_a',
    evidence: '__dirname = 0;',
    line: 1,
    character: 1,
    a: '__dirname',
    b: undefined,
    c: undefined,
    d: undefined,
    reason: 'Unexpected dangling \'_\' in \'__dirname\'.' },
  { id: '(error)',
    raw: '\'{a}\' was used before it was defined.',
    code: 'used_before_a',
    evidence: '__dirname = 0;',
    line: 1,
    character: 1,
    a: '__dirname',
    b: undefined,
    c: undefined,
    d: undefined,
    reason: '\'__dirname\' was used before it was defined.' } ]
> JSLINT('var __dirname = 0;', {node: true})
false
> JSLINT.data().errors
[ { id: '(error)',
    raw: 'Read only.',
    code: 'read_only',
    evidence: 'var __dirname = 0;',
    line: 1,
    character: 5,
    a: '__dirname',
    b: undefined,
    c: undefined,
    d: undefined,
    reason: 'Read only.' } ]
> JSLINT('var __dirname = 0;')
false
> JSLINT.data().errors
[ { id: '(error)',
    raw: 'Unexpected dangling \'_\' in \'{a}\'.',
    code: 'dangling_a',
    evidence: 'var __dirname = 0;',
    line: 1,
    character: 5,
    a: '__dirname',
    b: undefined,
    c: undefined,
    d: undefined,
    reason: 'Unexpected dangling \'_\' in \'__dirname\'.' } ]

If you diff the above outputs, you will find that JSLINT only stops reporting errors in the case of interfacing with __dirname. Everything else remains the same. Adding this exception doesn't open up a "loophole" whereby node developers can violate good nomenclature rules by declaring a local variable called __dirname; we still get "Read only.".

@gaborsar
Copy link

If this is true (nomen will be removed), than this will be a big problem for developers using MongoDB (_id) as well.

@jacksonrayhamilton
Copy link
Author

It seems like a broader solution might be to allow read-only access to identifiers with dangling underbars. That way, we could

  • still interface with codebases that [unfortunately] like to make-believe privacy,
  • utilize the _ identifier on its own (as in underscore, lodash),
  • and we can use identifiers where underbars were chosen to avoid naming conflicts, which I assume was MongoDB's motivation with the _id and __v fields. And probably the case for __dirname and __filename too.

There are still some issues with forbidding write access though:

  • If an API requires us to define properties with dangling underbars, as Node's stream.Duplex and stream.Transform do, then we have no choice but to drink the kool aid.
  • Executing a query in MongoDB, such as db.records.find( { "user_id": { $lt: 42} }, { "_id": 0, "name": 1 , "email": 1 } ), is also a no-go, because we need to assign 0 to _id.

It doesn't seem like there is a good work-around to this. I suppose it's partially our own fault for picking libraries that lack convention (if we even had a choice in the matter). It will be bad if nomen leaves because other people still insist on following silly (or not-so-silly) naming conventions.

If nomen is going to stay then this PR may be of less importance. This PR is a little selfish because it only solves one of the many exception-demanding situations I have described above. I will probably have to compromise with the rest of the world by turning the option on.

@jacksonrayhamilton
Copy link
Author

It looks like this has been fixed in the beta. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants