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

Breaking: change relative paths with --config (refs eslint/rfcs#37) #12887

Merged
merged 5 commits into from Mar 23, 2020
Merged

Conversation

mysticatea
Copy link
Member

@mysticatea mysticatea commented Feb 8, 2020

Prerequisites checklist

  • I have read the contributing guidelines.
  • The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

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

[X] Other, please explain: change some existing core features: fixes #11558.

What changes did you make? (Give an overview)

This PR implements RFC37 -- changes the relative paths of ignorePatterns, overrides[].files, and overrides[].excludedFiles as being relative to the current working directory if the config is provided by the --config option.

I'm sorry for these large differences. Most of the changes are to replace the pair of a name and a file path in parameters to a context object. Because most internal methods in ConfigArrayFactory had the pair in its parameter, the differences went huge. The context object has four properties; type, name, filePath, and matchBasePath. The matchBasePath is the new stuff to specify the base path of ignorePatterns, overrides[].files, and overrides[].excludedFiles. (the type was moved from config data because it's a context data rather than a config data.)

On the way, I found an unused option options.parent in some methods of ConfigArrayFactory and remove those.

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

Nothing in particular.

@mysticatea mysticatea added core accepted breaking feature labels Feb 8, 2020
@mysticatea mysticatea added this to Implemented, pending review in v7.0.0 Feb 8, 2020
@mysticatea mysticatea added this to Implemented, pending review in RFCs Feb 8, 2020
nzakas
nzakas approved these changes Mar 12, 2020
Copy link
Member

@btmills btmills left a comment

Great work piping this through, @mysticatea. I didn't spot any issues or missing cases, though there's a lot here, so I'd appreciate 👀 from more reviewers. I haven't wrapped my mind around the config loading/merging code as much as I would like to feel fully confident in reviewing it. That said, as you explained, I did get into a kind of rhythm seeing path, name replaced by ctx over and over. I appreciate that you collected that information in a single context object rather than adding an additional argument everywhere.

I'm always cautious about removing an option like parent without knowing why it was added in the first place and why it isn't used anymore, so I did some digging. It was added in #11546, and the only use at the time appears to be here, in CascadingConfigArrayFactory#_finalizeConfigArray:

finalConfigArray = configArrayFactory.loadInDirectory(
os.homedir(),
{ name: "PersonalConfig", parent: finalConfigArray }
);

That use was removed in #12678. That was enough for me, so I'm sharing my research in case anyone else had a similar question.

I had one minor wording change suggestion that occurred two places in the docs. Marking as approved because this LGTM either way, and I'll let you decide if you think it's an improvement.

docs/user-guide/configuring.md Outdated Show resolved Hide resolved
docs/user-guide/configuring.md Outdated Show resolved Hide resolved
try {
configData = loadConfigFile(ctx.filePath);
} catch (error) {
if (!error || error.code !== "ESLINT_CONFIG_FIELD_NOT_FOUND") {
Copy link
Member

@kaicataldo kaicataldo Mar 20, 2020

Choose a reason for hiding this comment

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

Not a blocker for this PR, but an idea I had while reviewing this: maybe we could centralize these errors codes as constants that can be imported?

Copy link
Member

@nzakas nzakas Mar 20, 2020

Choose a reason for hiding this comment

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

I’d suggest opening an issue for that idea so we can focus the conversation on this PR around what needs to happen to get this merged.

Copy link
Member Author

@mysticatea mysticatea Mar 23, 2020

Choose a reason for hiding this comment

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

For information, this constant value should appear in the userland only if --config path/to/a/package.json is present, the package.json exists, and eslintConfig field doesn't exist in the package.json. (We can distinguish between ENOENT and that by code, but it may be not important.)

mysticatea and others added 2 commits Mar 23, 2020
Co-Authored-By: Brandon Mills <btmills@users.noreply.github.com>
Co-Authored-By: Brandon Mills <btmills@users.noreply.github.com>
@mysticatea
Copy link
Member Author

@mysticatea mysticatea commented Mar 23, 2020

Thank you for your review. And I'm sorry for my late response. I have been back from the funeral of my grandma and around procedures.

@mysticatea mysticatea merged commit 48b122f into master Mar 23, 2020
11 checks passed
v7.0.0 automation moved this from Implemented, pending review to Done Mar 23, 2020
@mysticatea mysticatea deleted the rfc37 branch Mar 23, 2020
@mysticatea mysticatea moved this from Implemented, pending review to Done in RFCs Apr 25, 2020
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Sep 21, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age label Sep 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted archived due to age breaking core feature
Projects
No open projects
RFCs
  
Done
v7.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

4 participants