New: `no-global-assign` rule (fixes #6586) #6746

alberto · 2016-07-22T20:23:25Z

What issue does this pull request address?
#6586

What changes did you make? (Give an overview)
Duplicate and rename no-native-reassign to no-global-assign and mark the former as deprecated.

Is there anything you'd like reviewers to focus on?
Let me know if the deprecation messages are enough and if the commit message and tag are correct for this kind on change.

eslintbot · 2016-07-22T20:23:27Z

LGTM

mention-bot · 2016-07-22T20:23:27Z

@alberto, thanks for your PR! By analyzing the annotation information on this pull request, we identified @mysticatea, @jrajav and @nzakas to be potential reviewers

ilyavolodin · 2016-07-22T21:10:04Z

lib/rules/no-extend-native.js

@@ -1,6 +1,7 @@
 /**
 * @fileoverview Rule to flag adding properties to native object's prototypes.
 * @author David Nelson
+ * @deprecated in ESLint v3.2.0


Sorry, I don't remember what did we decide. Do we just leave the old rule unchanged, or should we remove all of the code and just do module.exports = require('./no-global-assign');? I guess former, since we don't want new changes to the rule to be propagated to deprecated version.

Yes, that's what we decided in #6586 (comment)

mysticatea · 2016-07-23T03:19:26Z

Oh, we should deprecate no-native-reassign rule, not no-extend-native rule.

mysticatea · 2016-07-23T03:26:13Z

Is the deprecated rule removed from rule list page?

alberto · 2016-07-23T09:00:51Z

Oh, we should deprecate no-native-reassign rule, not no-extend-native rule.

Oops! 😅

Is the deprecated rule removed from rule list page?

No. I don't think it should be removed since it's still available and actually part of eslint:recommended. I added (deprecated) to the description. We could also group the deprecated rules, similarly to the removed ones, but maybe we should open a new issue for that?

eslintbot · 2016-07-23T09:12:34Z

LGTM

mysticatea · 2016-07-23T20:28:10Z

Looks good to me. Thank you!

nzakas · 2016-07-25T18:10:52Z

We also need to figure out where to list deprecated rules and add a deprecation notice to the rule docs. Possibly, we should add this into the rule meta too. @ilyavolodin @pedrottimark thoughts on that?

nzakas · 2016-07-25T18:11:49Z

lib/rules/no-global-assign.js

+        docs: {
+            description: "disallow assignments to native objects or read-only global variables",
+            category: "Best Practices",
+            recommended: true


Not actually recommended, correct?

Oh, you are right

ilyavolodin · 2016-07-25T19:20:08Z

Yes, I think deprecated should become another meta property. I think @mysticatea already assumed as such with his PR for eslint:all

platinumazure · 2016-07-25T19:32:37Z

I'm 👍 on meta.deprecated unless that will raise some other problem I'm not aware of.

vitorbal · 2016-07-26T10:59:50Z

I would be willing to help out with this issue if no one minds. I also had
in my radar generating the deprecated rules list for the rules index page,
unless you're already on that, @ilyavolodin?
On Mon, Jul 25, 2016 at 9:32 PM Kevin Partington notifications@github.com
wrote:

I'm 👍 on meta.deprecated unless that will raise some other problem I'm
not aware of.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#6746 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAmNdnQYXLIy0_pAHjsWUOVNaAEIFxTKks5qZQ9XgaJpZM4JTGV0
.

ilyavolodin · 2016-07-26T13:21:56Z

@vitorbal No, it's all yours.

alberto · 2016-07-26T18:57:34Z

@nzakas do you want me to address the rules list generation as part of this PR? Or what do you intend to do @vitorbal?

eslintbot · 2016-07-26T22:57:48Z

LGTM

alberto · 2016-07-26T23:02:06Z

I have updated the PR with the suggestions above and added a new section deprecated in conf/category-list.json

@vitorbal now that I had a look at how the generation is done, I realized that maybe you referred to the changes in the site repo?

Let me know if there is anything else needed here

vitorbal · 2016-07-27T09:36:51Z

@alberto I think you actually got it covered now with your latest commits.
As for the site repo, I don't have much experience there yet, but I can give it a shot! From what I can see the only thing missing would be to add the list of deprecated rules in docs/rules/index.md?

edit: Ok, I think I figured it out. I will send a PR in the site repo for adding the list of deprecated rules in docs/rules/index.md!

vitorbal · 2016-07-27T12:51:36Z

conf/category-list.json

@@ -8,6 +8,13 @@
        { "name": "Stylistic Issues", "description": "These rules relate to style guidelines, and are therefore quite subjective:" },
        { "name": "ECMAScript 6", "description": "These rules relate to ES6, also known as ES2015:" }
    ],
+    "deprecated": {
+        "name": "Deprecated",
+        "description": "These rules have been deprecated and replaced by newer rules.",


For the "removed" rules we end the description with a colon instead of a dot. Should we do the same here for consistency?

nzakas · 2016-07-28T15:29:54Z

Makefile.js

@@ -209,6 +209,10 @@ function generateRuleIndexPage(basedir) {
    find(path.join(basedir, "/lib/rules/")).filter(fileType("js")).forEach(function(filename) {
        var rule = require(filename);

+        if (rule.meta.deprecated) {


Do we want to skip this entirely? Shouldn't there be a category for that?

We could add it to the categoriesData.deprecated.rules array here instead of doing it manually in https://github.com/eslint/eslint/pull/6746/files#r72645142

I think that is a good idea. Less manual work is always preferable.

eslintbot · 2016-07-29T17:33:57Z

LGTM

alberto · 2016-07-29T17:36:07Z

PR updated with suggestions from above.

eslintbot · 2016-07-29T21:45:33Z

LGTM

vitorbal · 2016-07-30T12:14:17Z

Lgtm

nzakas · 2016-08-01T19:18:11Z

LGTM

jquerybot added the CLA: Valid label Jul 22, 2016

ilyavolodin reviewed Jul 22, 2016
View reviewed changes

alberto changed the title ~~New: no-global-assign rule (fixes #6585)~~ New: no-global-assign rule (fixes #6586) Jul 23, 2016

alberto force-pushed the issue6585 branch from 2c9eaef to c4cef89 Compare July 23, 2016 09:12

btmills mentioned this pull request Jul 23, 2016

New: func-call-spacing rule (fixes #6080) #6749

Merged

mysticatea mentioned this pull request Jul 25, 2016

Update: make eslint:all excluding deprecated rules (fixes #6734) #6756

Merged

nzakas reviewed Jul 25, 2016
View reviewed changes

alberto force-pushed the issue6585 branch from c4cef89 to cc589ed Compare July 26, 2016 22:57

vitorbal reviewed Jul 27, 2016
View reviewed changes

vitorbal mentioned this pull request Jul 27, 2016

Show list of deprecated rules in rules index eslint/archive-website#277

Merged

nzakas reviewed Jul 28, 2016
View reviewed changes

mysticatea mentioned this pull request Jul 29, 2016

New: no-unsafe-negation rule (fixes #2716) #6789

Merged

alberto force-pushed the issue6585 branch from cc589ed to 9cfe7f0 Compare July 29, 2016 17:33

New: no-global-assign rule (fixes #6586)

f88180e

alberto force-pushed the issue6585 branch from 9cfe7f0 to f88180e Compare July 29, 2016 21:45

nzakas merged commit 2b17459 into master Aug 1, 2016

alberto deleted the issue6585 branch August 2, 2016 01:08

foxhound87 mentioned this pull request Aug 19, 2016

Move chai and path from dependencies to devDependencies foxhound87/mobx-react-form#7

Closed

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: `no-global-assign` rule (fixes #6586) #6746

New: `no-global-assign` rule (fixes #6586) #6746

alberto commented Jul 22, 2016 •

edited

eslintbot commented Jul 22, 2016

mention-bot commented Jul 22, 2016

ilyavolodin Jul 22, 2016

alberto Jul 23, 2016

mysticatea commented Jul 23, 2016

mysticatea commented Jul 23, 2016

alberto commented Jul 23, 2016 •

edited

eslintbot commented Jul 23, 2016

mysticatea commented Jul 23, 2016

nzakas commented Jul 25, 2016

nzakas Jul 25, 2016

alberto Jul 25, 2016

ilyavolodin commented Jul 25, 2016

platinumazure commented Jul 25, 2016

vitorbal commented Jul 26, 2016

ilyavolodin commented Jul 26, 2016

alberto commented Jul 26, 2016

eslintbot commented Jul 26, 2016

alberto commented Jul 26, 2016 •

edited

vitorbal commented Jul 27, 2016 •

edited

vitorbal Jul 27, 2016

nzakas Jul 28, 2016

vitorbal Jul 28, 2016

nzakas Jul 29, 2016

eslintbot commented Jul 29, 2016

alberto commented Jul 29, 2016

eslintbot commented Jul 29, 2016

vitorbal commented Jul 30, 2016

nzakas commented Aug 1, 2016

New: no-global-assign rule (fixes #6586) #6746

New: no-global-assign rule (fixes #6586) #6746

Conversation

alberto commented Jul 22, 2016 • edited

eslintbot commented Jul 22, 2016

mention-bot commented Jul 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mysticatea commented Jul 23, 2016

mysticatea commented Jul 23, 2016

alberto commented Jul 23, 2016 • edited

eslintbot commented Jul 23, 2016

mysticatea commented Jul 23, 2016

nzakas commented Jul 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyavolodin commented Jul 25, 2016

platinumazure commented Jul 25, 2016

vitorbal commented Jul 26, 2016

ilyavolodin commented Jul 26, 2016

alberto commented Jul 26, 2016

eslintbot commented Jul 26, 2016

alberto commented Jul 26, 2016 • edited

vitorbal commented Jul 27, 2016 • edited

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

eslintbot commented Jul 29, 2016

alberto commented Jul 29, 2016

eslintbot commented Jul 29, 2016

vitorbal commented Jul 30, 2016

nzakas commented Aug 1, 2016

New: `no-global-assign` rule (fixes #6586) #6746

New: `no-global-assign` rule (fixes #6586) #6746

alberto commented Jul 22, 2016 •

edited

alberto commented Jul 23, 2016 •

edited

alberto commented Jul 26, 2016 •

edited

vitorbal commented Jul 27, 2016 •

edited