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

add resolveConfigWithFilePath API #3846

Closed
wants to merge 2 commits into from
Closed

add resolveConfigWithFilePath API #3846

wants to merge 2 commits into from

Conversation

SavePointSam
Copy link
Contributor

This PR introduces a new API prettier.resolveConfigWithFilePath. This method uses the pre-existing internals to find and merge prettier configurations, but exposes the resolved file path for consumers. This will close #3451 and allow us to solve prettier/prettier-atom#270.

I also reorganized the API docs a bit so that similar API sit next to each other.

@azz
Copy link
Member

azz commented Jan 31, 2018

What if we just had resolveConfig return the path, too:

prettier.resolveConfig.sync('some/path.js');
// -> 
//    {
//      useTabs: true,
//      configPath: "/abs/path/.prettierrc"
//    }

@SavePointSam
Copy link
Contributor Author

That's also an option, however I would suggest against it. It makes sense that the API returns the configuration as Prettier can later ingest it. Adding extra properties in this one case doesn't quite make sense to me and feels like a mix of concerns. Even more so when the rest of the time we are working with the configuration payload it is separated between config and filePath.

@@ -52,6 +62,13 @@ function _resolveConfig(filePath, opts, sync) {
return null;
}

if (!!includeFilePath) {
Copy link
Member

Choose a reason for hiding this comment

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

Can this just be if (includeFilePath)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

docs/api.md Outdated
@@ -57,6 +57,18 @@ If `options.editorconfig` is `true` and an [`.editorconfig` file](http://editorc

Use `prettier.resolveConfig.sync(filePath [, options])` if you'd like to use sync version.

## `prettier.resolveConfigWithFilePath(filePath [, options])`

`resolveConfigWithFilePath` works that same as `resolveConfig` with one difference. The response will include both the configuration that was resolved, along with the file path to the configuration that was used to generate the configuration. However, it will not include the path to the located `.editorconfig` file when used as the core Prettier configuration acts as an override. This is useful for editor integrations, to subscribe to the resolved configuration and reload with any changes.
Copy link
Collaborator

@josephfrazier josephfrazier Feb 1, 2018

Choose a reason for hiding this comment

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

I'd suggest the following changes:

The response will include both the configuration that was resolved, along with the file path to the configuration file that was used to generate the configuration

However, it will not include the path to the located any .editorconfig file when used, as the core Prettier configuration configuration file acts as an override.

(note the , after used)

@josephfrazier
Copy link
Collaborator

...wait a minute. Couldn't we just export resolveConfigFile from src/config/resolve-config.js and index.js, instead of adding a separate function?

After typing this out, I realized that the use case (editor integrations) will likely want both the config and the file path anyway, so it probably makes sense to combine them into a single function, instead of forcing the integration to call two functions (which would also unnecessarily repeat the path resolution). I'm going to leave this comment here anyway, for the record.

@SavePointSam
Copy link
Contributor Author

I thought the same thing at first. Though as someone who would be implementing this type of functionality, I'd much rather make a call that returns both the resolved/merged config as well as the file path of the config used. There is room for error when using those API separately.

@olsonpm
Copy link

olsonpm commented Feb 3, 2018

I believe a valid workaround until this is resolved would be to use cosmiconfig's load directly. Anyway, I'm planning on using that to address "Show which options are used on format".

@azz
Copy link
Member

azz commented Feb 4, 2018

Is there a way to expose the path to the editorconfig file too?

@josephfrazier
Copy link
Collaborator

I don't think https://github.com/editorconfig/editorconfig-core-js provides a way to see the path of the files being parsed (note that there can be more than one). However, assuming there's just one, we could use some combination of find-up and find-project-root to locate it. See also #3559

@olsonpm
Copy link

olsonpm commented Feb 4, 2018

"assuming just one" could produce false positives though the simplification would be nice. Seems the devs behind editorconfig-core-js are pretty responsive though, so a PR adding a method such as this one wouldn't be out of the question.

@SavePointSam
Copy link
Contributor Author

Sorry for the silence! Having some crunch time at the office :(

I totally agree we should find a way to expose the path to .editorconfig. Can it actually pick up multiple to merge together? That's pretty rough..

@olsonpm
Copy link

olsonpm commented Feb 13, 2018

Bummer about the office time. I'm in between jobs so I finally get to work on stuff I enjoy haha.

Can it actually pick up multiple to merge together?

Unfortunately. I created a quick repo if you wanted to mess around with it.

@duailibe
Copy link
Member

duailibe commented Apr 26, 2018

FTR resolveConfigFile was added #4364

@suchipi
Copy link
Member

suchipi commented May 2, 2018

I'm going to close this based on #3846 (comment)

@suchipi suchipi closed this May 2, 2018
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 31, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose config.resolveConfigFile in API?
7 participants