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

Fix: remove dupe error message in bin/eslint (fixes #8964) #9320

Closed

Conversation

VictorHom
Copy link
Member

[X ] Other, please explain: remove the dupe error message which re-adds what was previously fixed in here

@eslintbot
Copy link

LGTM

@mention-bot
Copy link

@VictorHom, thanks for your PR! By analyzing the history of the files in this pull request, we identified @not-an-aardvark, @nzakas and @platinumazure to be potential reviewers.

@eslintbot
Copy link

LGTM

@not-an-aardvark not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features labels Sep 18, 2017
@not-an-aardvark
Copy link
Member

Thanks for the PR! Sorry about the delayed response.

The fix in #8965 was reverted as a result of #9011 (it was causing some error messages to not appear at all). Has that problem been solved? It seems like if we make only this change, the problem described in #9011 (comment) will reappear.

@VictorHom
Copy link
Member Author

@not-an-aardvark Can you recreate the issue? Im currently using:

(venv) ➜ eslint_sandbox node --version
v8.2.1
(venv) ➜ eslint_sandbox npm -version
5.3.0

With the latest eslint and with this change in the pr, I am not getting duplicated error messages.

Do you have a way to create case where no error message show at all?

@not-an-aardvark
Copy link
Member

I tested this by adding a space character to the beginning of the .eslintrc.yml file in the ESLint repository, (making it invalid YAML), then running bin/eslint.js Makefile.js.

On master, the output is:

Cannot read config file: /path/to/eslint/.eslintrc.yml
Error: end of the stream or a document separator is expected at line 3, column 1:
    plugins:
    ^
YAMLException: end of the stream or a document separator is expected at line 3, column 1:
    plugins:
    ^
    at generateError (/path/to/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:165:10)
    at throwError (/path/to/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:171:9)
    at readDocument (/path/to/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1512:5)
    at loadDocuments (/path/to/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1548:5)
    at load (/path/to/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1569:19)
    at Object.safeLoad (/path/to/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1591:10)
    at loadYAMLConfigFile (/path/to/eslint/lib/config/config-file.js:95:21)
    at loadConfigFile (/path/to/eslint/lib/config/config-file.js:231:22)
    at loadFromDisk (/path/to/eslint/lib/config/config-file.js:530:18)
    at Object.load (/path/to/eslint/lib/config/config-file.js:592:20)

On this branch, the output is:

YAMLException: end of the stream or a document separator is expected at line 3, column 1:
    plugins:
    ^
    at generateError (/path/to/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:165:10)
    at throwError (/path/to/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:171:9)
    at readDocument (/path/to/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1512:5)
    at loadDocuments (/path/to/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1548:5)
    at load (/path/to/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1569:19)
    at Object.safeLoad (/path/to/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1591:10)
    at loadYAMLConfigFile (/path/to/eslint/lib/config/config-file.js:95:21)
    at loadConfigFile (/path/to/eslint/lib/config/config-file.js:231:22)
    at loadFromDisk (/path/to/eslint/lib/config/config-file.js:530:18)
    at Object.load (/path/to/eslint/lib/config/config-file.js:592:20)

Note that the line at the top ("Cannot read config file: /path/to/eslint/.eslintrc.yml") is missing when using this branch.

Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

Temporarily requesting changes due to #9320 (comment) to ensure that this doesn't get accidentally merged before we figure out the missing error messages.

@kaicataldo
Copy link
Member

Where do we stand with this?

@kaicataldo
Copy link
Member

Friendly ping!

@VictorHom VictorHom closed this May 26, 2018
@VictorHom VictorHom deleted the pr/bin_eslint_dupe_err_mssg branch May 26, 2018 22:16
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Nov 24, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Nov 24, 2018
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 bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants