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

valid-jsdoc: validate that scalar types are written in lowercase #3056

Closed
janmarek opened this Issue Jul 17, 2015 · 30 comments

Comments

Projects
None yet
9 participants
@janmarek
Copy link

janmarek commented Jul 17, 2015

I personally write number, string, boolean and function types in lowercase and Object and Array with first capital letter. I would appreciate if it could be validated.

@param {number} num
@return {Object} 
...
@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Jul 17, 2015

Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.

Reporting a bug? Please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. The source code that caused the problem
  3. The configuration you're using (for the rule or your entire config file)
  4. The actual ESLint output complete with line numbers

Requesting a new rule? Please be sure to include:

  1. The use case for the rule - what is it trying to prevent or flag?
  2. Whether the rule is trying to prevent an error or is purely stylistic
  3. Why you believe this rule is generic enough to be included

Requesting a feature? Please be sure to include:

  1. The problem you want to solve (don't mention the solution)
  2. Your take on the correct solution to problem

Including this information in your issue helps us to triage it and get you a response as quickly as possible.

Thanks!

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Aug 12, 2015

@eslint/eslint-team thoughts on this?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Aug 12, 2015

I like it!

@nzakas nzakas added enhancement rule accepted and removed triage labels Aug 12, 2015

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Aug 12, 2015

I think this needs to be configurable in some way. Some people would use object and some would use Object.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Aug 13, 2015

Object would not be checked by this option.

I do think it needs to be configurable. For instance, i use Function not function because functions are objects.

@janmarek

This comment has been minimized.

Copy link
Author

janmarek commented Aug 13, 2015

I agree it should be configurable.

@jaydenseric

This comment has been minimized.

Copy link

jaydenseric commented Nov 30, 2015

This would be super useful, because I am constantly wondering what case to use for certain types. Actual JSDoc documentation is inconsistent in their examples.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Nov 30, 2015

I don't know if the JSDoc spec cares one way or the other, but even for "number" I could see arguments both ways (constructor is "Number", typeof output is "number"). So unless JSDoc itself proscribes one way or the other, I don't think ESLint should (that is, it should allow for both ways via configuration).

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 1, 2015

@platinumazure it doesn't matter what JSDoc does, we can still apply additional constraints. We just need to make them configurable.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Dec 1, 2015

@nzakas Agreed. What I was saying is, we shouldn't apply un-configurable
constraints unless JSDoc clearly proscribes said constraints.
On Dec 1, 2015 10:04 AM, "Nicholas C. Zakas" notifications@github.com
wrote:

@platinumazure https://github.com/platinumazure it doesn't matter what
JSDoc does, we can still apply additional constraints. We just need to make
them configurable.


Reply to this email directly or view it on GitHub
#3056 (comment).

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 1, 2015

Yup. That's what I said here: #3056 (comment)

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Dec 28, 2015

I am planning to work on this. Any recommendation on how the option would be configured?

Option 1
Define preferences for the standard types and they will have defaults.

{
    "string": "string",
    "object": "Object",
    "number": "Number"
}

Option 2
Set it to true to make it start with capital letter

{
    "string": false,
    "object": true,
    "number": true
}
@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 29, 2015

I think option 1 is right, maybe called preferType?

@gyandeeps gyandeeps self-assigned this Dec 29, 2015

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Dec 29, 2015

Here is the default I am thinking of

var PREFER_TYPE_DEFAULTS = {
    "string": "string",
    "object": "object",
    "number": "Number",
    "function": "Function",
    "boolean": "boolean",
    "int": "Int",
    "array": "Array"
};

Please suggests corrections for the above.

Should we start checking types using the defaults for this rule or would this be a opt-in feature where only the option specified by the user would be checked?

CC @nzakas @eslint/eslint-team

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Dec 29, 2015

I like lowercase for types which are gotten by typeof operator -- boolean, number, string, object, function, symbol, and undefined.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 29, 2015

@gyandeeps Why not just pass in an array of they values as you'd like them to appear in JSDoc? Something like this:
[2, { preferType: ["string", "object", "Number", "Function", "boolean", "Int", "Array"]} ]

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jan 1, 2016

Anything that isn't an object should be lowercase, so "boolean" and "int"

"Object" and "Function" should be uppercase because they are objects.

@ilyavolodin what does an array of strings mean in this case?

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Jan 1, 2016

Array of string the way they should be seen in JSDoc comments. If array contains "Object" that means anytime "object" is found, it should warn. Simpler then duplicating every word i.e. { "object": "Object" }

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jan 2, 2016

You're talking about a whitelist, which I don't believe is feasible. There's no way users will constantly update this list for every type they expect to see (which includes custom classes). The blacklist approach is more reasonable for this use case.

@jaydenseric

This comment has been minimized.

Copy link

jaydenseric commented Jan 2, 2016

@ilyavolodin I like the simple array idea. @nzakas It could still be a blacklist, by doing a case-insensitive lookup. If found, the result is then compared to the test value case-sensitive. If not found, the test value is compared against the JSDoc default or ignored.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Jan 3, 2016

@jaydenseric Right, that's exactly what I mean. Do case insensitive lookup and then fail if the case-sensitive lookup fails.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jan 3, 2016

Not sure I follow, can you give an example?

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Jan 3, 2016

Given a jsDoc like this:

/**
 * @param {string} string The string to parse.
 */

And a configuration like this:

[2, { preferType: ["String", "object", "Number", "Function", "boolean", "Int", "Array"]} ]

The code can parse argument type from the param and do a case-insensitive lookup from the preferType array. It will get location 0, then it can do a case-sensitive comparison of the type to preferType[0] and if they are not the same, error out.

@janmarek

This comment has been minimized.

Copy link
Author

janmarek commented Jan 3, 2016

@ilyavolodin This configuration format looks good to me.

@jaydenseric

This comment has been minimized.

Copy link

jaydenseric commented Jan 4, 2016

This might be a good guide for capitalization defaults: Google JavaScript Style Guide, JavaScript Types section. In particular, look at the Type Example column in the table under Types in JavaScript.

Interestingly, number and Number are considered seperate, equally valid values with different meanings. We couldn't lint capitalization for such values.

@janmarek

This comment has been minimized.

Copy link
Author

janmarek commented Jan 4, 2016

Number/String objects are very rare. I've personally never used any of them. Validation with exception for these types would be useless.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jan 4, 2016

@ilyavolodin I'm still having a hard time with this, and the automatic conversion to all lowercase seems a bit magical. I still think being able to say "if you see x then it should be y" is more extensible and explicit than the single array, and follows conventions we already use.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Jan 4, 2016

@nzakas I totally agree that it's more explicit, it's just that you need to repeat yourself a lot, and seems a bit redundant. But I'm fine either way.

gyandeeps added a commit that referenced this issue Jan 4, 2016

gyandeeps added a commit that referenced this issue Jan 4, 2016

gyandeeps added a commit that referenced this issue Jan 4, 2016

@tschaub

This comment has been minimized.

Copy link
Contributor

tschaub commented Jan 5, 2016

One nuance specific to the Closure Compiler is that Function and function(number) have specific meaning: the former is any function, the latter is a function that takes a single number argument.

In addition, in this strict types environment, the following are valid values for the given types:

  • number : 10, NaN
  • Number : new Number(10), null
  • string : 'foo'
  • String : new String('foo'), null
  • boolean : true
  • Boolean : new Boolean(true), null
  • Object : {}, null
  • Array : [], null

So I don't think the preferType in 563d97d is going to work in this situation.

I think it would be unfortunate if ESLint's valid-jsdoc rule were strict in a way that made it incompatible with type checking.

@tschaub

This comment has been minimized.

Copy link
Contributor

tschaub commented Jan 5, 2016

So I don't think the preferType in 563d97d is going to work in this situation.

Actually, I take that back. This would work:

preferTypes: {
  array: 'Array',
  object: 'Object',
}

The linter would warn about typos in those two cases.

gyandeeps added a commit that referenced this issue Jan 7, 2016

@nzakas nzakas closed this in 5a633bf Jan 13, 2016

nzakas added a commit that referenced this issue Jan 13, 2016

Merge pull request #4855 from eslint/issue3056
Update: Add `preferType` option to `valid-jsdoc` rule (fixes #3056)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.