add number-literal-case rule - fixes #55 #57

SamVerschueren · 2016-10-17T06:55:59Z

This rule implements #55. Not sure about the description though.

SamVerschueren · 2016-10-17T07:02:33Z

Also not sure about the implementation when I read @sindresorhus his comment

You just need to tell ESLint that you want to listen to any ExpressionStatement. You then ensure node.expression.type === 'Literal', and you can check its .raw property

That didn't work. Maybe ESLint changed something? Or Sindre didn't drink his morning coffee before writing that :).

sindresorhus · 2016-10-17T08:10:52Z

That didn't work. Maybe ESLint changed something? Or Sindre didn't drink his morning coffee before writing that :).

☕️🙉 You're right.

sindresorhus

Looks really good!

sindresorhus · 2016-10-17T08:12:40Z

docs/rules/number-literal-case.md

+const foo = 0B11;
+
+const foo = 0O10;
+```


I think the separation between the types might be clearer as different code-blocks than newlines.

sindresorhus · 2016-10-17T08:13:18Z

docs/rules/number-literal-case.md

@@ -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.


convention in => convention of

sindresorhus · 2016-10-17T08:16:08Z

docs/rules/number-literal-case.md

+# 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.
+


We should include a reasoning here. Maybe:

Differentiating the casing of the identifier and value clearly separates them and makes your code more readable.

Improvement suggestions welcome.

sindresorhus · 2016-10-17T08:17:40Z

index.js

@@ -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'


@jfmengels Should we do the same as the AVA plugin here?

Not sure what you mean?

sindresorhus · 2016-10-17T08:18:57Z

docs/rules/number-literal-case.md

@@ -0,0 +1,27 @@
+# Enforce a lowercase literal identifier and an uppercase value


Enforce a lowercase identifier and uppercase value for number literals

?

That's better. I thought the same about adding number literal somewhere in that sentence.

Drop the a too:

Enforce lowercase identifier and uppercase value for number literals

sindresorhus · 2016-10-17T08:20:24Z

rules/number-literal-case.js

+			}
+		},
+		fixable: 'code'
+	},


We don't really use the meta property. It's not used by ESLint, but the ESLint team uses it for their docs.

@jfmengels ?

Oh, good to know. I thought that it was used internally or something. I'll drop it.

sindresorhus · 2016-10-17T08:31:31Z

rules/number-literal-case.js

+	}
+
+	const indicator = value[1].toLowerCase();
+	const val = value.substring(2).toUpperCase();


I generally prefer .slice() when possible as it's more common. @jfmengels Maybe we should have a rule about that?

I rarely use either of those. I personally have to experiment with or find the docs for slice every time I use it, while substring is more self-explanatory.
But I have a weak opinion on this. Feel free to write a proposal

sindresorhus · 2016-10-17T08:33:28Z

Congrats on your first ESLint rule, Sam!

SamVerschueren · 2016-10-17T10:14:47Z

Congrats on your first ESLint rule, Sam!

Thanks! I enjoyed it to be honest :). Glad I did it.

jfmengels · 2016-10-17T13:17:00Z

I can't find the comment about the meta, but I started using it in my other plugins, to:

Generate my recommended config
Generate my README with all the rules

There is talk in eslint-find-rules to use the deprecated field in core rules and plugins, and there's talk of using that field in plugin like eslint-plugin-react and import.

Short to say: I like the meta object, and I think I'll use it more and more.

Great work for a first rule @SamVerschueren, and glad you liked it :)

jfmengels · 2016-10-17T13:21:29Z

test/number-literal-case.js

+		'const foo = 0xFF',
+		'const foo = 0b11',
+		'const foo = 0o10',
+		`const foo = '0Xff'`


This case does not respect the title of the rule Enforce lowercase identifier and uppercase value for number literals.

Sure, the casing is not the same, but I thought we were enforcing lowercase then uppercase.

@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 raw value of a string is "'0Xff'". With this test, I just wanted to make sure this is not enforced on string values. Does it make sense somehow :p?

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. 👍

sindresorhus · 2016-10-17T16:46:17Z

@jfmengels If we're gonna use the meta object, I think that's a separate discussion and we should do it to all the rules at once and wait until we actually use a thing that needs it, like your recommended workflow generator. So I think we should leave it out in this PR. Probably better to do the discussion in eslint-plugin-ava first, as we have more eyes there.

jfmengels · 2016-10-17T19:24:02Z

If we're gonna use the meta object, I think that's a separate discussion and we should do it to all the rules at once

Absolutely. Will create an issue about it in the AVA plugin repo.

And thanks @SamVerschueren! :D

jfmengels · 2016-10-18T08:42:46Z

Opened an issue at avajs/eslint-plugin-ava#151

add number-literal-case rule - fixes sindresorhus#55

c55fd47

sindresorhus requested changes Oct 17, 2016

View reviewed changes

sindresorhus reviewed Oct 17, 2016

View reviewed changes

process feedback

db51b49

SamVerschueren added 2 commits October 17, 2016 12:15

remove meta

9128981

fix description

6249057

jfmengels suggested changes Oct 17, 2016

View reviewed changes

sindresorhus approved these changes Oct 17, 2016

View reviewed changes

jfmengels merged commit 718578a into sindresorhus:master Oct 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add number-literal-case rule - fixes #55 #57

add number-literal-case rule - fixes #55 #57

SamVerschueren commented Oct 17, 2016

SamVerschueren commented Oct 17, 2016

sindresorhus commented Oct 17, 2016

sindresorhus left a comment

sindresorhus Oct 17, 2016

sindresorhus Oct 17, 2016

sindresorhus Oct 17, 2016

sindresorhus Oct 17, 2016

jfmengels Oct 17, 2016

sindresorhus Oct 17, 2016

SamVerschueren Oct 17, 2016

sindresorhus Oct 17, 2016

sindresorhus Oct 17, 2016

SamVerschueren Oct 17, 2016

sindresorhus Oct 17, 2016

jfmengels Oct 17, 2016

sindresorhus commented Oct 17, 2016

SamVerschueren commented Oct 17, 2016

jfmengels commented Oct 17, 2016 •

edited

Loading

jfmengels Oct 17, 2016

SamVerschueren Oct 17, 2016

jfmengels Oct 17, 2016

sindresorhus commented Oct 17, 2016

jfmengels commented Oct 17, 2016

jfmengels commented Oct 18, 2016

		@@ -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.

add number-literal-case rule - fixes #55 #57

add number-literal-case rule - fixes #55 #57

Conversation

SamVerschueren commented Oct 17, 2016

SamVerschueren commented Oct 17, 2016

sindresorhus commented Oct 17, 2016

sindresorhus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Oct 17, 2016

SamVerschueren commented Oct 17, 2016

jfmengels commented Oct 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Oct 17, 2016

jfmengels commented Oct 17, 2016

jfmengels commented Oct 18, 2016

jfmengels commented Oct 17, 2016 •

edited

Loading