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 rule no-deprecated #182

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

abhirathore2006
Copy link

@abhirathore2006 abhirathore2006 commented May 9, 2018

Adding support no-deprecated rule to make migration to newer version easier

@ganimomer
Copy link
Contributor

Hey, and thanks for contributing!
This rule could help a lot with migration once Lodash v5 comes out, but I think it's a bit too specific.
Since the plugin already contains a curated list of Lodash functions (or close enough that changing them is a feasible goal), I think a better rule would be something like no-deprecated, that reports for every method that doesn't exist in a specified version.
(e.g. your declared version in the plugin could be version 4, but you could set the rule to version 5)

@abhirathore2006
Copy link
Author

@ganimomer thanks for advice, i will make appropriate changes

@abhirathore2006 abhirathore2006 changed the title add rule no-omit add rule no-deprecated May 11, 2018
@abhirathore2006
Copy link
Author

@ganimomer i updated the rule to no-deprecated

Copy link
Contributor

@ganimomer ganimomer left a comment

Choose a reason for hiding this comment

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

Great work!
I made some comments about the spec and implementation, which I think should be changed a bit.


This rule takes two arguments.

* severity
Copy link
Contributor

Choose a reason for hiding this comment

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

In all other rules, we don't count severity as an argument.

@@ -0,0 +1,15 @@
# Prevent use of deperectaed or future deprecations
Copy link
Contributor

Choose a reason for hiding this comment

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

You have a typo in deprecated.

This rule takes two arguments.

* severity
* array of lodash versions e.g. [4,5]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this is an array.
Whoever uses this rule probably wants to migrate to a specific version, so they can probably input their destination version, and the rule can figure out which deprecations to include by itself.
e.g. If a user uses v3 (which we can get by the version global config, usually supplied by using a config), and they want to upgrade to v5, they can just write 5 and the rule can figure out to include both 4 and 5 as deprecations.

@@ -0,0 +1,34 @@
'use strict'
module.exports = {
Copy link
Contributor

Choose a reason for hiding this comment

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

The plugin already has a methodDataByVersion files. Maybe there's a way to introduce this data there, so all the information is in the same place?
Most of the information is already there - any method that exists in 3 and doesn't exist in 4 is deprecated.
This means we'd have to add a 5.js already, but assuming you can run the master version of Lodash in Node, you can probably use a script to generate a file for it automatically. You don't even need to add all the information yet (that's subject to change until v5 is released probably). You can just use empty object for each method.
This might not include the recommendation part, but it's neater, I think.

bind: 'use built-in functions',
bindAll: 'use built-in functions',
rest: 'use built-in functions',
spread: 'use built-in functions',
Copy link

Choose a reason for hiding this comment

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

rest/spread are built-in syntax (not functions)

Copy link
Author

Choose a reason for hiding this comment

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

@jdalton i will update it, i didn't miss any deperecation right ?

Copy link

Choose a reason for hiding this comment

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

I think it's a good start. It can always be added to as v5 develops.

@abhirathore2006
Copy link
Author

@ganimomer i am thinking to add deprecations list in methodDataByVersion files .

{
 _deprecations:{
    key:"message"
  }
}

@ganimomer
Copy link
Contributor

@abhirathore2006 Sounds good, but why the _?

@abhirathore2006
Copy link
Author

@ganimomer to distinguish it from other properties, which are the names of actual functions

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.

3 participants