add resolveConfigWithFilePath API#3846
add resolveConfigWithFilePath API#3846SavePointSam wants to merge 2 commits intoprettier:masterfrom SavePointSam:issue-3451_resolve-config-with-file
Conversation
|
What if we just had prettier.resolveConfig.sync('some/path.js');
// ->
// {
// useTabs: true,
// configPath: "/abs/path/.prettierrc"
// } |
|
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 |
| return null; | ||
| } | ||
|
|
||
| if (!!includeFilePath) { |
There was a problem hiding this comment.
Can this just be if (includeFilePath)?
|
|
||
| ## `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. |
There was a problem hiding this comment.
I'd suggest the following changes:
The response will include both the configuration that was resolved, along with the
filepath to theconfigurationfile that was used to generate the configuration
However, it will not include the path to
the locatedany.editorconfigfilewhenused, as thecore Prettier configurationconfiguration file acts as an override.
(note the , after used)
|
...wait a minute. Couldn't we just export 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. |
|
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. |
|
I believe a valid workaround until this is resolved would be to use cosmiconfig's |
|
Is there a way to expose the path to the editorconfig file too? |
|
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 |
|
"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. |
|
Sorry for the silence! Having some crunch time at the office :( I totally agree we should find a way to expose the path to |
|
Bummer about the office time. I'm in between jobs so I finally get to work on stuff I enjoy haha.
Unfortunately. I created a quick repo if you wanted to mess around with it. |
|
FTR |
|
I'm going to close this based on #3846 (comment) |
lock
Bot
added
the
locked-due-to-inactivity
7 participants
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.