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: Reworking/improving optionator configuration for --print-config #7206

Merged
merged 1 commit into from Oct 7, 2016

Conversation

platinumazure
Copy link
Member

@platinumazure platinumazure commented Sep 21, 2016

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

[ ] Documentation update
[ ] 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
[x] Other, please explain:

Changing behavior of a CLI option (--print-config).

Please check each item to ensure your pull request is ready:

  • I've read the pull request guide
  • I've included tests for my change
  • (n/a) I've updated documentation for my change (if appropriate)

What changes did you make? (Give an overview)

Since --print-config operates on one file, I've changed the option from being boolean-valued to being string-valued (in other words, the file for which user wants to print a config becomes an argument to the option itself).

This is technically a breaking change because this use is now broken: eslint --print-config --other-options-here fileName.js. In addition, it will no longer flag if too many files are passed in. However, the advantage of this is the optionator output for this option is improved and we get validation for free. Consensus seems to be that this is not a breaking change, because --print-config --some-option fileName.js was never really intended to work, it just happened to because of the implementation.

Needed to update optionator to pick up a bugfix (otherwise, non-boolean options at the end of a command line were not validated properly).

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

Nothing in particular.

@eslintbot
Copy link

Thanks for the pull request, @platinumazure! I took a look to make sure it's ready for merging and found some changes are needed:

  • The commit summary must be 72 characters or shorter. Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

@mention-bot
Copy link

@platinumazure, thanks for your PR! By analyzing the annotation information on this pull request, we identified @christophercrouzet, @mysticatea and @gyandeeps to be potential reviewers

@eslintbot
Copy link

LGTM

@platinumazure platinumazure added cli Relates to ESLint's command-line interface breaking This change is backwards-incompatible evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Sep 21, 2016
@ilyavolodin
Copy link
Member

I don't think it's breaking, it's a bug fix. We always intended for print-config to work on a file, it was just not enforced before (bug).

@@ -137,6 +137,18 @@ const cli = {

log.info("v" + require("../package.json").version);

} else if (currentOptions.printConfig) {
Copy link
Member

Choose a reason for hiding this comment

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

What happens if I do this:

$ eslint --print-config foo.js bar.js

In our current implementation, we get a warning. If I'm reading this code correctly, the new version would print the config for foo.js and not warn at all about bar.js, which I think is misleading. We probably want to check that files.length is zero, and if not, throw up the same warning we currently do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I thought about that. You are right, the behavior has changed. So I can add that check back in. (I removed a unit test for that specific behavior. I guess I hadn't considered that use case important, oops.)

I'll add a check and I'll get that test back into tests/lib/cli.js. Sound good?

@platinumazure
Copy link
Member Author

platinumazure commented Sep 22, 2016

@eslint/eslint-tsc Just want to confirm, should this be regarded as a bug-fix and non-breaking, per @ilyavolodin's comment here? Thanks!

@eslintbot
Copy link

LGTM

@platinumazure
Copy link
Member Author

@nzakas I've added the logic and test back in, though I changed the error message for clarity. Please let me know if further changes are needed. Thanks!

Copy link
Member

@ilyavolodin ilyavolodin left a comment

Choose a reason for hiding this comment

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

LGTM

type: "Boolean",
description: "Print the configuration to be used"
type: "path::String",
description: "Print the configuration which would be used for linting a given file (linting will not occur)"
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing this will cause the text to wrap when output on the console. I'm not sure this needs to be changed from the previous text, but if it does, then can you make it a bit shorter?

@platinumazure
Copy link
Member Author

It does wrap, but prettily (I.e., first character of second line aligns
with first character of first line). I don't think it necessarily needs to
be shrunk, if you were thinking the wrapping would be ugly and go to the
left side of the console. Still want this shrunk?

On Sep 23, 2016 1:43 PM, "Nicholas C. Zakas" notifications@github.com
wrote:

@nzakas requested changes on this pull request.

In lib/options.js
#7206 (review):

@@ -216,8 +216,8 @@ module.exports = optionator({
},
{
option: "print-config",

  •        type: "Boolean",
    
  •        description: "Print the configuration to be used"
    
  •        type: "path::String",
    
  •        description: "Print the configuration which would be used for linting a given file (linting will not occur)"
    

I'm guessing this will cause the text to wrap when output on the console.
I'm not sure this needs to be changed from the previous text, but if it
does, then can you make it a bit shorter?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7206 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARWeiUyF5KgKb_3kbIKj1o0XX9l68VXks5qtB3CgaJpZM4KDXga
.

@platinumazure
Copy link
Member Author

platinumazure commented Sep 24, 2016

@nzakas Here's how my eslint --help looks with this change:

eslint-help

Personally I think this should work, but let me know if you want it shorter anyway.

@nzakas
Copy link
Member

nzakas commented Sep 26, 2016

I still think it should be shorter. We have documentation for details, these should just be short phrases.

@platinumazure
Copy link
Member Author

@nzakas Could I get a suggestion of what you're getting at? (Note that there are other options which go onto two lines.) I'm not sure how to shorten what I've got without losing meaning.

@gkz
Copy link
Contributor

gkz commented Sep 27, 2016

Optionator does nice line wrapping at all widths. That being said, you could rephrase it to something like:

Print the config to be used when linting the given file (linting won't occur)

@nzakas
Copy link
Member

nzakas commented Oct 1, 2016

@platinumazure I think you can just leave the message as it was before. I don't see a great need for changing it.

@eslintbot
Copy link

LGTM

@platinumazure platinumazure changed the title Breaking: Reworking/improving optionator configuration for --print-config Fix: Reworking/improving optionator configuration for --print-config Oct 6, 2016
@platinumazure platinumazure added bug ESLint is working incorrectly and removed breaking This change is backwards-incompatible labels Oct 6, 2016
@platinumazure
Copy link
Member Author

platinumazure commented Oct 6, 2016

@nzakas I've changed the option description-- it's shorter, but I wanted to at least allude to the option argument. It fits on one line on my terminal (which is 80 characters wide). I've also reclassified this as a non-breaking bugfix per consensus on this issue. Please let me know if anything else needs to change.

Also, please let me know if I still need to do anything to get this accepted? I think TSC can just accept if they like? (Nobody has objected to the idea yet, at any rate.)

@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Oct 7, 2016
@nzakas nzakas merged commit c9a7ec5 into master Oct 7, 2016
@platinumazure platinumazure deleted the rework-print-config branch October 7, 2016 18:06
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 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 Feb 6, 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 cli Relates to ESLint's command-line interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants