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

Chore: Add test for valid non-string input #463

Merged
merged 1 commit into from Dec 18, 2020

Conversation

stonegray
Copy link
Contributor

Test that buffers and strings produce equal output to resolve uncovered line.

@eslint-deprecated
Copy link

Hi @stonegray!, 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'.

Read more about contributing to ESLint here

@eslint-deprecated
Copy link

Hi @stonegray!, 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'.

Read more about contributing to ESLint here

@btmills
Copy link
Member

btmills commented Dec 16, 2020

Wow, recursive git blame traces that uncovered string coercion code all the way back to jquery/esprima@8e3d670 in 2012. Quite the history lesson! Given this behavior has been around longer than espree itself, I think we should test it as-is. Before accepting it unilaterally, I'll give the rest of the team a chance to chime in. I could be persuaded that we'd rather remove the coercion and say we officially only accept string inputs. That'd be a breaking change to be safe, but we have a major version coming up soon anyway.

@nzakas
Copy link
Member

nzakas commented Dec 16, 2020

I don’t think we need to make any changes, I’m fine with just adding the test.

@anikethsaha
Copy link
Member

Yeah , adding this test seems fine. additionally if we want to inform users, then we can add a note in docs saying its recommended to use string as inputs.

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.

Seems we're in agreement. Thanks @stonegray! LGTM.

@btmills btmills merged commit 3b4ca9e into eslint:master Dec 18, 2020
stonegray added a commit to stonegray-forks/espree that referenced this pull request Dec 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants