Skip to content

New: add --env-info flag to CLI#12270

Merged
kaicataldo merged 10 commits intomasterfrom
info-flag
Sep 25, 2019
Merged

New: add --env-info flag to CLI#12270
kaicataldo merged 10 commits intomasterfrom
info-flag

Conversation

@kaicataldo
Copy link
Copy Markdown
Member

@kaicataldo kaicataldo commented Sep 16, 2019 •
edited
Loading

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
[x] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

fixes #11958

What changes did you make? (Give an overview)
#12060 appears to have stalled, so I took some time to clean up my original proof of concept and make a PR! This feels like a win for issue triaging, since we can ask for the output of this command in our bug issue templates.

Is there anything you'd like reviewers to focus on?
Nothing in particular.

@eslint-deprecated eslint-deprecated Bot added the triage An ESLint team member will look at this issue soon label Sep 16, 2019
@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion cli Relates to ESLint's command-line interface feature This change adds a new feature to ESLint and removed triage An ESLint team member will look at this issue soon labels Sep 16, 2019
platinumazure
platinumazure suggested changes Sep 16, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

This looks great in general. Thanks for putting this together!

My only thought is that I'm not sure I'm a fan of the module name info. What info, exactly? Environment info? Rule info? Some other info? I think a name like environment-info or runtime-info would be a lot better even if it's a little more verbose.

@kaicataldo
Copy link
Copy Markdown
Member Author

kaicataldo commented Sep 16, 2019

runtime-info makes sense to me!

@kaicataldo
Copy link
Copy Markdown
Member Author

kaicataldo commented Sep 17, 2019

@platinumazure Updated 😃

@kaicataldo
Copy link
Copy Markdown
Member Author

kaicataldo commented Sep 17, 2019 •
edited
Loading

I've also added some regression tests with Windows-style paths. There isn't any OS-specific path handling logic yet, but thought it would be a good idea to ensure that future changes work with both styles of paths.

@kaicataldo
Copy link
Copy Markdown
Member Author

kaicataldo commented Sep 17, 2019

Hmmm the Windows tests pass locally on my Windows machine but are failing in CI. Not sure why that is - something to do with the check to see if the executing version is global or not. I've removed that commit for now and will see if I can figure out what's going on. I do think these tests are adequate.

@kaicataldo
Copy link
Copy Markdown
Member Author

kaicataldo commented Sep 17, 2019

In the spirit of @platinumazure's comment, I'm wondering if we should actually call the flag --env-info. info is very generic, and this flag outputs specific information. It would probably be best to reserve info for a more fitting CLI flag. Thoughts?

@kaicataldo kaicataldo changed the title New: add --info flag to CLI New: add --env-info flag to CLI Sep 17, 2019
platinumazure
platinumazure approved these changes Sep 17, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@platinumazure platinumazure 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!

@jamesgeorge007 jamesgeorge007 mentioned this pull request Sep 22, 2019
Closed
jamesgeorge007
jamesgeorge007 suggested changes Sep 22, 2019
View reviewed changes
Comment thread lib/shared/runtime-info.js
Comment thread lib/shared/runtime-info.js
Comment thread tests/lib/shared/runtime-info.js
Comment thread tests/lib/shared/runtime-info.js
jamesgeorge007
jamesgeorge007 reviewed Sep 22, 2019
View reviewed changes
Comment thread lib/shared/runtime-info.js
@kaicataldo
Copy link
Copy Markdown
Member Author

kaicataldo commented Sep 25, 2019

Thanks for the reviews! I'm going to go ahead and merge this.

@kaicataldo kaicataldo merged commit a7894eb into master Sep 25, 2019
@kaicataldo kaicataldo deleted the info-flag branch September 25, 2019 20:45
This was referenced Oct 8, 2019
Merged
Merged
Closed
This was referenced Oct 22, 2019
Merged
Open
This was referenced Nov 9, 2019
Open
Open
@snyk-bot snyk-bot mentioned this pull request Nov 21, 2019
Merged
@snyk-bot snyk-bot mentioned this pull request Feb 27, 2020
Merged
@a-martynovich a-martynovich mentioned this pull request Mar 10, 2020
Merged
This was referenced Mar 11, 2020
Merged
Open
@eslint-deprecated eslint-deprecated Bot locked and limited conversation to collaborators Mar 24, 2020
@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 Mar 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@platinumazure platinumazure platinumazure approved these changes

@ilyavolodin ilyavolodin ilyavolodin approved these changes

@jamesgeorge007 jamesgeorge007 jamesgeorge007 requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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 cli Relates to ESLint's command-line interface feature This change adds a new feature to ESLint

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[cli]: Have a dedicated option to show up local environment information

4 participants

@kaicataldo @platinumazure @ilyavolodin @jamesgeorge007