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: message.line could be undefined #191

Merged
merged 3 commits into from
Sep 12, 2021
Merged

Fix: message.line could be undefined #191

merged 3 commits into from
Sep 12, 2021

Conversation

JounQin
Copy link
Contributor

@JounQin JounQin commented Aug 26, 2021 •
edited

@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 mentioned this pull request Aug 26, 2021
Closed
nzakas
nzakas requested changes Aug 27, 2021
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.

Nice catch, though I think there’s a cleaner way of catching this error.

lib/processor.js Outdated Show resolved Hide resolved
JounQin
JounQin commented Aug 27, 2021
View reviewed changes
lib/processor.js Outdated Show resolved Hide resolved
nzakas
nzakas approved these changes Aug 28, 2021
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. Thanks.

@nzakas nzakas requested a review from btmills August 28, 2021 00:33
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 28, 2021 •
edited

CLA Signed

The committers are authorized under a signed CLA.

btmills added a commit to eslint/eslint that referenced this pull request Sep 6, 2021
@btmills
Docs: LintMessage.line and column are possibly undefined
7a19d4a 
In `lib/linter/linter.js`, the `parse()` function's exception handler
translates the exception's message to a `LintMessage`. It also passes
through the exception's `lineNumber` and `column`, but the exception is
not guaranteed to have those properties. In that case,
`LintMessage.line` and `column` can be `undefined`.

In eslint/eslint-plugin-markdown#191, the processor would crash when
attempting to map messages without a line.

Normally widening a type like this would be a breaking change, but since
this is only updating the docs to reflect reality, I think this can be a
semver-patch change.
@btmills btmills mentioned this pull request Sep 6, 2021
Merged
1 task
btmills
btmills reviewed Sep 6, 2021
View reviewed changes
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.

Good find, @JounQin. I agree the early return from adjustMessage() is the right fix, and I have one idea that could improve the experience with how we're adjusting messages without line information. What do you think about it?

I also noticed that ESLint's LintMessage @typedef and Node.js API docs say line and column are always numbers, so I opened eslint/eslint#15032 to add undefined as a possible value.

lib/processor.js Outdated Show resolved Hide resolved
@eslint-github-bot
Copy link

Hi @JounQin!, thanks for the Pull Request

The pull request title 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

1 similar comment
@eslint-github-bot
Copy link

Hi @JounQin!, thanks for the Pull Request

The pull request title 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: message.line could be undefined Fix: message.line could be undefined Sep 7, 2021
btmills
btmills reviewed Sep 7, 2021
View reviewed changes
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.

The implementation LGTM, and I had an idea that'll help the test do a better job protecting us from accidentally breaking this in some future change.

tests/lib/processor.js Outdated Show resolved Hide resolved
@JounQin @btmills
Update tests/lib/processor.js
a8f1b69 
Co-authored-by: Brandon Mills <btmills@users.noreply.github.com>
btmills
btmills approved these changes Sep 7, 2021
View reviewed changes
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 @JounQin! I’ll leave this open for a day or two in case someone else wants to review and then merge and release it.

btmills added a commit to eslint/eslint that referenced this pull request Sep 10, 2021
@btmills
Docs: LintMessage.line and column are possibly undefined (#15032)
91e82f5 
In `lib/linter/linter.js`, the `parse()` function's exception handler
translates the exception's message to a `LintMessage`. It also passes
through the exception's `lineNumber` and `column`, but the exception is
not guaranteed to have those properties. In that case,
`LintMessage.line` and `column` can be `undefined`.

In eslint/eslint-plugin-markdown#191, the processor would crash when
attempting to map messages without a line.

Normally widening a type like this would be a breaking change, but since
this is only updating the docs to reflect reality, I think this can be a
semver-patch change.
@btmills btmills merged commit 3a40160 into eslint:main Sep 12, 2021
@JounQin JounQin deleted the fix/line branch September 12, 2021 01:12
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

@btmills btmills btmills approved these changes

Assignees
No one assigned
Labels
triage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@JounQin @nzakas @btmills