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

Update: Add preferType option to valid-jsdoc rule (fixes #3056) #4855

Merged
merged 1 commit into from Jan 13, 2016

Conversation

gyandeeps
Copy link
Member

@nzakas @ilyavolodin This is what I am thinking when it comes to this enhancement.

CC: @janmarek
Please share your views on the approach and the option setup.

@gyandeeps gyandeeps added the do not merge This pull request should not be merged yet label Jan 5, 2016
@@ -188,6 +188,85 @@ By default ESLint requires you to specify `type` for `@return` tag for every doc
}]
```

#### preferType

It will validate all the types from jsdoc with the options setup by the user. Inside the options, key should be what the type you want to check and the value of it should be what the expected type should be. Please make all the keys in lowercase as we compare them by making the type from jsdoc to lowercase and the value should be case sensitive as in how you expect it to be.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can simplify this by treating it the same as prefer. So, the keys represent types you don't want to see and the values are what should be used instead. You could do { String: "string" } to warn about usng uppercase, for example. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what if inside the code somebody writes strinG by mistake then we will not be able to catch it. That why I always want to compare the lowercased value from the jsdoc with the key here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, good point. I'm just concerned that the conversion to lowercase will be confusing, and will prevent people from using both String and string while avoiding strinG.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think the unwanted: wanted pattern is sensible. Then prefer is about correcting wrongly chosen types (not typos). The toLowerCase conversion only catches a limited set of typos anyway. So it would guard against strinG but not sring etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even with unwanted: wanted pattern, how would we catch sring? As the matching against the keys will never find sring.
To make this work we need to maintain a whitelist of all the types that can be used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always say that this doesn't prevent misspellings, only common errors like using String instead of string.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will make the change and mention in docs that we dont check for spelling mistakes.

@gyandeeps gyandeeps removed the do not merge This pull request should not be merged yet label Jan 11, 2016
@gyandeeps gyandeeps changed the title WIP: Add preferType option to valid-jsdoc rule (fixes #3056) Update: Add preferType option to valid-jsdoc rule (fixes #3056) Jan 11, 2016
@nzakas
Copy link
Member

nzakas commented Jan 13, 2016

Lgtm

nzakas added a commit that referenced this pull request Jan 13, 2016
Update: Add `preferType` option to `valid-jsdoc` rule (fixes #3056)
@nzakas nzakas merged commit c01016a into master Jan 13, 2016
@gyandeeps gyandeeps deleted the issue3056 branch January 13, 2016 16:11
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants