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

Update: lint code block with same extension but different content #14227

Merged
merged 2 commits into from Apr 13, 2021
Merged

Update: lint code block with same extension but different content #14227

merged 2 commits into from Apr 13, 2021

Conversation

JounQin
Copy link
Contributor

@JounQin JounQin commented Mar 18, 2021

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?

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Mar 18, 2021
@eslint-github-bot
Copy link

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.

  • The commit message tag must be one of the following:

    The Tag is one of the following:

    • Fix - for a bug fix.
    • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
    • New - implements a new feature.
    • Breaking - for a backwards-incompatible enhancement or feature.
    • Docs - changes to documentation only.
    • Build - changes to build process only.
    • Upgrade - for a dependency upgrade.
    • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

    You can use the labels of the issue you are working on to determine the best tag.

  • There should be a space following the initial tag and colon, for example 'New: Message'.

  • The first letter of the tag should be in uppercase

Read more about contributing to ESLint here

@JounQin JounQin changed the title fix: lint code block with same extension but different content Fix: lint code block with same extension but different content Mar 18, 2021
// 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.");
Copy link
Member

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).

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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?

Copy link
Contributor Author

@JounQin JounQin Apr 9, 2021

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/eslint-plugin-markdown#183.

Copy link
Member

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.

Copy link
Contributor Author

@JounQin JounQin Apr 9, 2021

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.

Copy link
Member

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.

@mdjermanovic mdjermanovic added breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features and removed triage An ESLint team member will look at this issue soon labels Mar 19, 2021
@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion enhancement This change enhances an existing feature of ESLint and removed breaking This change is backwards-incompatible labels Mar 31, 2021
@nzakas
Copy link
Member

nzakas commented Mar 31, 2021

Per #14207, this issue has been accepted and there’s consensus that this change can be done in a minor version.

@nzakas
Copy link
Member

nzakas commented Apr 7, 2021

What’s the next step on this PR?

tests/lib/eslint/eslint.js Outdated Show resolved Hide resolved
tests/lib/eslint/eslint.js Outdated Show resolved Hide resolved
@JounQin JounQin requested a review from mdjermanovic Apr 9, 2021
Copy link
Member

@mdjermanovic mdjermanovic left a 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.

@mdjermanovic mdjermanovic changed the title Fix: lint code block with same extension but different content Update: lint code block with same extension but different content Apr 9, 2021
@nzakas nzakas requested a review from btmills Apr 10, 2021
Copy link
Member

@btmills btmills left a 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.

@btmills btmills merged commit 41b3570 into eslint:master Apr 13, 2021
13 checks passed
@JounQin JounQin deleted the fix/code_block branch Apr 13, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Oct 11, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Oct 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

overrides with processor works unexpectedly
4 participants