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

New: ecmaFeatures.impliedStrict (fixes: #227) #228

Merged
merged 1 commit into from Jan 5, 2016
Merged

Conversation

nre
Copy link
Contributor

@nre nre commented Jan 3, 2016

  • New feature ecmaFeatures.impliedStrict.
  • README.
  • Two tests.
  • Modified tests/lib/ecma-features.js:
    • ecmaFeatures[feature] = true throws for impliedStrict.

@nre nre force-pushed the issue227 branch 2 times, most recently from 3504dae to b6f5b60 Compare January 3, 2016 13:59
@@ -75,17 +75,18 @@ describe("ecmaFeatures", function() {
// Uncomment and fill in filename to focus on a single file
// var filename = "jsx/invalid-matching-placeholder-in-closing-tag";
var feature = path.dirname(filename),
permissiveValue = feature !== "impliedStrict",
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Member

Choose a reason for hiding this comment

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

Ooh, I think you're doing this because the implied strict tests are failing. Please revert this change and fix the tests.

@nre
Copy link
Contributor Author

nre commented Jan 3, 2016

The reason I changed ecma-features.js is that I don't know any errors that are silenced by strict mode. In other words, what code will make a 'non-strict' parser throw, but not a 'strict' parser? So for impliedStrict I swapped the 'correct/error' roles of the true/false feature settings.

The other ecmaFeatures,

  • jsx
  • globalReturn
  • experimentalObjectRestSpread

all solve parser errors when they are true. But impliedStrict finds more errors when it is true.

The impliedStrict tests generate the ASTs in the result files when impliedStrict: false, and throw when impliedStrict: true. This is the opposite of the other ecmaFeatures tests, but I don't think that means they are failing.

@nzakas
Copy link
Member

nzakas commented Jan 4, 2016

Ah, I see, thanks for explaining. We should split out that case in a more obvious way, then. Maybe create an array called failOnEnable and put impliedStrict in it?

@nre
Copy link
Contributor Author

nre commented Jan 5, 2016

I moved the condition into shouldThrowInTestsWhenEnabled(feature).

@nzakas
Copy link
Member

nzakas commented Jan 5, 2016

Fair enough, thanks!

nzakas added a commit that referenced this pull request Jan 5, 2016
New: ecmaFeatures.impliedStrict (fixes: #227)
@nzakas nzakas merged commit 78bf74f into eslint:master Jan 5, 2016
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.

None yet

2 participants