-
-
Notifications
You must be signed in to change notification settings - Fork 392
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 number-literal-case rule - fixes #55 #57
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
# Enforce a lowercase literal identifier and an uppercase value | ||
|
||
Enforces a convention in defining number literals where the literal identifier is written in lowercase and the value in uppercase. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. convention in => convention of |
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should include a reasoning here. Maybe:
Improvement suggestions welcome. |
||
|
||
## Fail | ||
|
||
```js | ||
const foo = 0XFF; | ||
const foo = 0xff; | ||
const foo = 0Xff; | ||
|
||
const foo = 0B11; | ||
|
||
const foo = 0O10; | ||
``` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the separation between the types might be clearer as different code-blocks than newlines. |
||
|
||
|
||
## Pass | ||
|
||
```js | ||
const foo = 0xFF; | ||
|
||
const foo = 0b11; | ||
|
||
const foo = 0o10; | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,8 @@ module.exports = { | |
'unicorn/filename-case': ['error', {case: 'kebabCase'}], | ||
'unicorn/no-abusive-eslint-disable': 'error', | ||
'unicorn/no-process-exit': 'error', | ||
'unicorn/throw-new-error': 'error' | ||
'unicorn/throw-new-error': 'error', | ||
'unicorn/number-literal-case': 'error' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jfmengels Should we do the same as the AVA plugin here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure what you mean? |
||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
'use strict'; | ||
const fix = value => { | ||
if (!/^0[a-zA-Z]/.test(value)) { | ||
return value; | ||
} | ||
|
||
const indicator = value[1].toLowerCase(); | ||
const val = value.substring(2).toUpperCase(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I generally prefer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I rarely use either of those. I personally have to experiment with or find the docs for |
||
|
||
return `0${indicator}${val}`; | ||
}; | ||
|
||
const create = context => { | ||
return { | ||
Literal: node => { | ||
const value = node.raw; | ||
const fixedValue = fix(value); | ||
|
||
if (value !== fixedValue) { | ||
context.report({ | ||
node, | ||
message: 'Invalid number literal casing', | ||
fix: fixer => fixer.replaceText(node, fixedValue) | ||
}); | ||
} | ||
} | ||
}; | ||
}; | ||
|
||
module.exports = { | ||
meta: { | ||
meta: { | ||
docs: { | ||
description: 'Enforce a lowercase literal identifier and an uppercase value', | ||
recommended: 'error' | ||
} | ||
}, | ||
fixable: 'code' | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't really use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, good to know. I thought that it was used internally or something. I'll drop it. |
||
create | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
import test from 'ava'; | ||
import avaRuleTester from 'eslint-ava-rule-tester'; | ||
import rule from '../rules/number-literal-case'; | ||
|
||
const ruleTester = avaRuleTester(test, { | ||
env: { | ||
es6: true | ||
}, | ||
parserOptions: { | ||
sourceType: 'module' | ||
} | ||
}); | ||
|
||
const error = { | ||
ruleId: 'number-literal-case', | ||
message: 'Invalid number literal casing' | ||
}; | ||
|
||
ruleTester.run('number-literal-case', rule, { | ||
valid: [ | ||
'const foo = 0xFF', | ||
'const foo = 0b11', | ||
'const foo = 0o10', | ||
`const foo = '0Xff'` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This case does not respect the title of the rule Sure, the casing is not the same, but I thought we were enforcing lowercase then uppercase. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jfmengels It's a string that I test here. When I created the rule, I was on my bike thinkering about this "should I explicitely test if the literal is of type number". This is not needed as the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, my bad! Didn't look closely enough. That's a good test to have, as you often get surprising results when you're writing tests like that. 👍 |
||
], | ||
invalid: [ | ||
{ | ||
code: 'const foo = 0XFF', | ||
errors: [error], | ||
output: 'const foo = 0xFF' | ||
}, | ||
{ | ||
code: 'const foo = 0xff', | ||
errors: [error], | ||
output: 'const foo = 0xFF' | ||
}, | ||
{ | ||
code: 'const foo = 0Xff', | ||
errors: [error], | ||
output: 'const foo = 0xFF' | ||
}, | ||
{ | ||
code: 'const foo = 0Xff', | ||
errors: [error], | ||
output: 'const foo = 0xFF' | ||
}, | ||
{ | ||
code: 'const foo = 0B11', | ||
errors: [error], | ||
output: 'const foo = 0b11' | ||
}, | ||
{ | ||
code: 'const foo = 0O10', | ||
errors: [error], | ||
output: 'const foo = 0o10' | ||
}, | ||
{ | ||
code: ` | ||
const foo = 255; | ||
|
||
if (foo === 0xff) { | ||
console.log('invalid'); | ||
} | ||
`, | ||
errors: [error], | ||
output: ` | ||
const foo = 255; | ||
|
||
if (foo === 0xFF) { | ||
console.log('invalid'); | ||
} | ||
` | ||
} | ||
] | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's better. I thought the same about adding
number literal
somewhere in that sentence.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop the
a
too: