Optimze both either description #2368
Conversation
adispring
force-pushed
the
optimze-both-either-description
branch
from
b70fe7a
to
99cf77c
Compare
|
@adispring You have a merge conflict ;) |
|
Definitely a good idea. I'm still triaging issues, but if no one has gotten around to it before I get a chance, I'll clean up the conflicts. |
|
The conflict has been fixed. |
|
@adispring would you mind fixing the linter errors as well? |
|
Ok, I'll see what happened. |
|
There is a eslint error in test/test.js, so I have to add a babel-eslint parser to fix this problem: https://stackoverflow.com/questions/51936079/eslint-how-to-ignore-parsing-error-invalid-regular-expression-flag |
.eslintrc
Outdated
| @@ -7,6 +7,7 @@ | |||
| "mocha": true, | |||
| "es6": true | |||
| }, | |||
| "parser": "babel-eslint", | |||
There was a problem hiding this comment.
Are you sure this is necessary? I didn't see any linter errors in your last push; only a failing test. So I am bit confused here.
There was a problem hiding this comment.
As this stackoverflow said, regex symbol s is a ecmascript 2018 feature, eslint's default parser can not parse /***/s to a valid AST, and can not lint it.
There's 2 ways to fix this:
- Add
"ecmaVersion": 6to .eslintrc, but this will cause other eslint problems related to ecam version. - add babel-eslint parser to parse
/***/s.
There was a problem hiding this comment.
Sorry I must be missing something here.
I know that #3158 introduced the s flag but last time I checked your pull request failed for exactly the same reason master is failing right now:
This is the only test that is failing on master:
I'm failing to see how this relates to the regex flag.
That same test used to fail on your pull request however you have now chosen to disable that test.
There was a problem hiding this comment.
What I am trying to say is that your pr didn't fail because of that regex flag you mention but because of that unit test failure.
There was a problem hiding this comment.
Ok ignore this last comment. I just figured out that adding that babel-eslint was intended to fix a lint issue. But it does seem that we can fix that lint error without depending on another third-party plugin, we can just set the parser options in the eslint config file. See my other comment.
There was a problem hiding this comment.
So I spent some time trying to figure out the issue with that test and I think that the test simply can't work at all considering the implementation of swap. I opened a PR to fix the linter and tests failures so that this PR can focus on just the documentation. See #3224
package.json
Outdated
| "devDependencies": { | ||
| "@babel/cli": "^7.4.4", | ||
| "@babel/core": "^7.4.5", | ||
| "@babel/preset-env": "^7.7.4", | ||
| "@babel/register": "^7.4.4", | ||
| "@babel/types": "^7.4.4", | ||
| "@rollup/plugin-babel": "^5.0.4", | ||
| "babel-eslint": "^10.1.0", |
There was a problem hiding this comment.
I played with the .eslintrc config and found that we don't need a plugin to fix the s regex flag issue. This seems to work fine too:
"parserOptions": {
"sourceType": "module",
"ecmaVersion": 2018 <-- HERE
},
|
@adispring: With #3224 merged only this commit of yours should be necessary now: 99cf77c I can help you rebasing this branch (and others) if you don't have the time right now. |
adispring
force-pushed
the
optimze-both-either-description
branch
from
f21e035
to
0426e96
Compare
|
Just reset to the first PR commit. @customcommander |
No description provided.