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

Jump to bottom

New: pass cwd from cli engine #12389

Merged
merged 2 commits into from Oct 16, 2019
Merged

New: pass cwd from cli engine #12389

merged 2 commits into from Oct 16, 2019

Conversation

fa93hws
Copy link
Contributor

@fa93hws fa93hws commented Oct 8, 2019

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

What changes did you make? (Give an overview)
Pass the cwd from the cli engine
#11218
RFC link: eslint/rfcs#35

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

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Oct 8, 2019
fa93hws
fa93hws commented Oct 8, 2019
View reviewed changes
lib/linter/linter.js Outdated Show resolved Hide resolved
kaicataldo
kaicataldo suggested changes Oct 8, 2019
View reviewed changes
Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

I think the new Linter config object should be optional so that it’s not a breaking change (and we don’t have to add that empty object everywhere).

Do you mind updating the documentation as well?

lib/linter/linter.js Outdated Show resolved Hide resolved
lib/linter/linter.js Outdated Show resolved Hide resolved
lib/linter/linter.js Outdated Show resolved Hide resolved
mysticatea
mysticatea suggested changes Oct 9, 2019
View reviewed changes
Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!
I have some suggestions. Would you address those?

lib/linter/linter.js Outdated Show resolved Hide resolved
lib/linter/linter.js Outdated Show resolved Hide resolved
mysticatea
mysticatea reviewed Oct 9, 2019
View reviewed changes
lib/linter/linter.js Outdated Show resolved Hide resolved
mysticatea
mysticatea suggested changes Oct 10, 2019
View reviewed changes
Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

Thank you for the quick update!

Almost LGTM, but I have a few suggestions.

docs/developer-guide/nodejs-api.md Outdated Show resolved Hide resolved
lib/linter/linter.js Outdated Show resolved Hide resolved
lib/linter/linter.js Outdated Show resolved Hide resolved
@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint and removed triage An ESLint team member will look at this issue soon labels Oct 10, 2019
kaicataldo
kaicataldo suggested changes Oct 10, 2019
View reviewed changes
Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

Some suggestions around documentation. Otherwise, this LGTM. Thanks for working on this!

docs/developer-guide/nodejs-api.md Outdated Show resolved Hide resolved
docs/developer-guide/nodejs-api.md Outdated Show resolved Hide resolved
docs/developer-guide/nodejs-api.md Outdated Show resolved Hide resolved
docs/developer-guide/working-with-rules.md Outdated Show resolved Hide resolved
tests/lib/linter/linter.js Outdated Show resolved Hide resolved
docs/developer-guide/nodejs-api.md Outdated Show resolved Hide resolved
@kaicataldo
Copy link
Member

kaicataldo commented Oct 10, 2019 •
edited

Ah, also forget to mention: can you change the commit message from Chore: to Enhancement: New:? Since this adds a new API, it should be a semver-minor change.

Edited: @mysticatea Whoops, yes! Thank you for catching that.

@mysticatea
Copy link
Member

@kaicataldo I guess you meant Update: or New:.

@kaicataldo kaicataldo added feature This change adds a new feature to ESLint and removed enhancement This change enhances an existing feature of ESLint labels Oct 10, 2019
@fa93hws fa93hws changed the title Chore: pass cwd from cli engine New: pass cwd from cli engine Oct 11, 2019
kaicataldo
kaicataldo approved these changes Oct 12, 2019
View reviewed changes
Copy link
Member

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

mysticatea
mysticatea approved these changes Oct 14, 2019
View reviewed changes
Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@kaicataldo kaicataldo merged commit 874fe16 into eslint:master Oct 16, 2019
@kaicataldo
Copy link
Member

Thanks for contributing!

@fa93hws fa93hws deleted the pass-cwd-from-cli-engine branch October 16, 2019 00:07
@fa93hws fa93hws mentioned this pull request Oct 16, 2019
Closed
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 Dec 1, 2019
Merged
@snyk-bot snyk-bot mentioned this pull request Feb 13, 2020
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
This was referenced Mar 26, 2020
Merged
Merged
Merged
Open
This was referenced Apr 3, 2020
Merged
Merged
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Apr 15, 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 Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mysticatea mysticatea mysticatea approved these changes

@kaicataldo kaicataldo kaicataldo approved these changes

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 core Relates to ESLint's core APIs and features 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.

None yet
3 participants
@fa93hws @kaicataldo @mysticatea