Conversation
I didn't add the full description because it looks wrong to me, but I can't investigate that further now. See eslint/eslint#10188
@@ -4,7 +4,6 @@ | |||
RegExp('['); | |||
//#Err: no-invalid-regexp | |||
RegExp('.', 'z'); | |||
//#Err: no-invalid-regexp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should still report an error according to eslint website https://eslint.org/docs/rules/no-invalid-regexp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, it actually should throw an error, but it doesn't. I opened an issue in the eslint repository: eslint/eslint#10861
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to merge this PR in and then when that fix is in we can make another PR for the minor version update. Because technically this is the way eslint 5.6.0 works right now. When this is fixed itll be a diff version. Unless this PR is meant to cover all eslint rules from 5.6.0 - 5.7.0?
Also, FYI I will be fixing this week eslint/eslint#10861
Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sstern6 I would prefer to wait a bit more to avoid a regression in the product. It can lead to weird cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I understand, so then when that fix is merged in codacy will support eslint 5.6.x with the updated fix and will dependent on Eslint next release? How will subsequent releases but handled with eslint. Will a new PR be created in this repo for every minor or major version?
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally every release we do a new PR here to update the version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sounds good @rtfpessoa. PR for this issue has been merged in eslint/eslint#10920. Im guessing this will make it into the 5.6.2 release, so we will either need to update this PR to support 5.6.2 and label it asa such. Or merge this in and make another PR for 5.6.2.
Given our previous conversation I assume you would like to update this PR?
I just would not like to merge this one since it disables a test. I would
prefer to fix it first.
…On Tue, 9 Oct 2018, 6:13 pm Scott Stern, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/main/resources/docs/tests/no-invalid-regexp.js
<codacy/codacy-eslint#103 (comment)>:
> @@ -4,7 +4,6 @@
RegExp('[');
//#Err: no-invalid-regexp
RegExp('.', 'z');
-//#Err: no-invalid-regexp
Ok sounds good @rtfpessoa <https://github.com/rtfpessoa>. PR for this
issue has been merged in eslint/eslint#10920
<eslint/eslint#10920>. Im guessing this will make
it into the 5.6.2 release, so we will either need to update this PR to
support 5.6.2 and label it asa such. Or merge this in and make another PR
for 5.6.2.
Given our previous conversation I assume you would like to update this PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<codacy/codacy-eslint#103 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA3E8HnycJPeF_2mRxgdrZfjKj2936LOks5ujNkjgaJpZM4Wp7-6>
.
|
@rtfpessoa eslint 5.7.0 has been released with the fix that you requested. @FloEdelmann would you mind updating this PR and name to support 5.7.0? https://eslint.org/blog/2018/10/eslint-v5.7.0-released Does that sound ok to everyone? |
@sstern6 Done. Thanks for your updates on this! |
@FloEdelmann should I continue the update or will you take care of it? Edit: noticed you already updated. Will fix the test issue. |
And add all rules that were added to eslint since version 4.15.0.
I hope that everything still works, given that it's a breaking version change. As far as I can tell, the breaking changes shouldn't have an impact on the usage here.
See also https://eslint.org/docs/user-guide/migrating-to-5.0.0 and https://github.com/eslint/eslint/releases.