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.

Sign up for GitHub

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

Jump to bottom

fix!: Parsing 'exported' comment using parseListConfig #17675

Merged
merged 3 commits into from Dec 22, 2023
Merged

fix!: Parsing 'exported' comment using parseListConfig #17675

merged 3 commits into from Dec 22, 2023

Conversation

amondev
Copy link
Contributor

@amondev amondev commented Oct 22, 2023

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[X] Other, please explain:

Parsing 'exported' comment using parseListConfig.
Fixes #17622.

What changes did you make? (Give an overview)

In the docs, only information about the name entries.
However, when processing /* exported */ comments, Linter uses parseStringConfig function, making it appear as if it's checking the name: value entries.
So, change Linter to use the parseListConfig function for parsing /* exported */ comments.

Is there anything you'd like reviewers to focus on?

@amondev amondev requested a review from a team as a code owner October 22, 2023 12:25
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 22, 2023 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

@eslint-github-bot eslint-github-bot bot added the chore This change is not user-facing label Oct 22, 2023
@netlify
Copy link

netlify bot commented Oct 22, 2023 •
edited

✅ Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 1ad5823
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/657554211c60ad0008a0e6d2

@mdjermanovic mdjermanovic changed the title refactor: Parsing 'exported' comment using parseListConfig fix!: Parsing 'exported' comment using parseListConfig Oct 22, 2023
@eslint-github-bot eslint-github-bot bot added breaking This change is backwards-incompatible bug ESLint is working incorrectly labels Oct 22, 2023
@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features and removed chore This change is not user-facing labels Oct 22, 2023
@nzakas nzakas marked this pull request as draft October 23, 2023 15:09
@nzakas
Copy link
Member

nzakas commented Oct 23, 2023

@amondev thanks so much for contributing. I think we need to decide on this point before proceeding: #17622 (comment)

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 2, 2023

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Nov 2, 2023
@Rec0iL99 Rec0iL99 removed the Stale label Nov 3, 2023
@github-actions GitHub Actions
Copy link

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Nov 13, 2023
@Rec0iL99 Rec0iL99 removed the Stale label Nov 17, 2023
@github-actions GitHub Actions
Copy link

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Nov 27, 2023
@Rec0iL99
Copy link
Member

refer #17622 (comment).

@Rec0iL99 Rec0iL99 removed the Stale label Nov 27, 2023
@injae-kim
Copy link

#17622 (comment)

FYI) Now updating this PR with @amondev , sorry for late! 🙇

@amondev amondev marked this pull request as ready for review December 5, 2023 12:32
@mdjermanovic mdjermanovic marked this pull request as draft December 6, 2023 12:18
@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Dec 6, 2023
@mdjermanovic
Copy link
Member

I converted this PR to draft, as we keep breaking changes as drafts for now.

mdjermanovic
mdjermanovic reviewed Dec 6, 2023
View reviewed changes
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.

The code changes look good to me.

Can you also add some tests to confirm the new behavior?

  • /* exported foo, bar */ - should mark both variables.
  • /* exported foo bar */ - shouldn't mark any.
  • /* esported foo: true */ - also shouldn't mark.

Tests for SourceCode could be after this one, tests for Linter eslintrc mode inside this describe, test for Linter flat config mode inside this describe.

@amondev
Copy link
Contributor Author

amondev commented Dec 7, 2023

@mdjermanovic Thank you for your comment! I'll attempt to add a test.

@amondev
Copy link
Contributor Author

amondev commented Dec 11, 2023

@mdjermanovic I updated PR! 😄

@github-actions GitHub Actions
Copy link

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Dec 21, 2023
@nzakas nzakas marked this pull request as ready for review December 21, 2023 23:39
nzakas
nzakas approved these changes Dec 21, 2023
View reviewed changes
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Waiting for @mdjermanovic to verify his suggestions.

mdjermanovic
mdjermanovic approved these changes Dec 22, 2023
View reviewed changes
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.

LGTM, thanks for contributing!

@mdjermanovic mdjermanovic merged commit 200518e into eslint:main Dec 22, 2023
22 checks passed
bmish added a commit to bmish/eslint that referenced this pull request Dec 27, 2023
@bmish
Merge branch 'main' into rfc-100-part2
0aebc8a 
* main:
  chore: use jsdoc/no-multi-asterisks with allowWhitespace: true (eslint#17900)
  chore: fix getting scope in tests (eslint#17899)
  fix!: Parsing 'exported' comment using parseListConfig (eslint#17675)
  docs: updated examples of `max-lines` rule (eslint#17898)
  feat!: Remove valid-jsdoc and require-jsdoc (eslint#17694)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nzakas nzakas nzakas approved these changes

@mdjermanovic mdjermanovic mdjermanovic approved these changes

Assignees
No one assigned
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion breaking This change is backwards-incompatible bug ESLint is working incorrectly contributor pool core Relates to ESLint's core APIs and features
Projects
Triage
Status: Complete
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change Request: /* exported */ should not allow values
5 participants
@amondev @nzakas @Rec0iL99 @injae-kim @mdjermanovic