-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Update: lint code block with same extension but different content #14227
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
Conversation
Hi @JounQin!, thanks for the Pull Request The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.
Read more about contributing to ESLint here |
// Resolve configuration again if the file extension was changed. | ||
if (configForRecursive && path.extname(blockName) !== originalExtname) { | ||
debug("Resolving configuration again because the file extension was changed."); | ||
// Resolve configuration again if the file content or extension was changed. | ||
if (configForRecursive && (text !== blockText || path.extname(blockName) !== originalExtname)) { | ||
debug("Resolving configuration again because the file content or extension was changed."); |
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 is potentially a breaking change. The current behavior when preprocess()
returns the same extension isn't well documented, but maybe some processors don't expect that they'll be run again on a part of content that originates from the same file. This also changes how existing user configurations will be interpreted (#14207).
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 there any example that this change will break some processors? I can't imagine that.
And It's just the correct behavior.
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.
And by referencing my original issue, I don't understand what will change, I tried this change locally, rules of *.foo/*
(including *.foo/*.foo
) take higher priority than *.foo
, it is very expected to me.
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.
I agree with you that the current behavior looks unexpected, but we can't really know all third-party processors to tell if this will break something. I marked this PR as "breaking" until we figure out whether the original issue is a bug or just an undocumented exception in the behavior, and estimate the impact of a fix if it's a bug.
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.
Then how do you figure out whether it's a bug?
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.
I see part 2 as a bug fix that will break at least one processor and possibly others.
Is it still an open question whether we will fix only 1, or both 1 and 2?
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.
As @btmills mentioned, foo.md/0_0.markdown/0_0.js
is already been linted now, so 2) should be fixed too.
Besides, this issue has been fixed by eslint/markdown#183.
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.
Yes, I noticed that eslint-plugin-markdown
has been fixed and released, I'm just concerned about the "possibly others" part.
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.
That issue is not caused by this fix at all, and also md -> html -> js
, so I don't think we should concern about that part.
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.
I agree. If other processors have the same implementation bug as eslint-plug-in-markdown
, it's already possible to repro with a different syntax in between, so this is making it slightly easier to encounter in the worst case.
Per #14207, this issue has been accepted and there’s consensus that this change can be done in a minor version. |
What’s the next step on this PR? |
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 LGTM but would like someone else to verify.
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.
LGTM. Thanks for all the processor improvements recently @JounQin! This can be merged once we've confirmed that Friday's release won't require a follow-up patch release.
close #14207
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
As title
Is there anything you'd like reviewers to focus on?