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.

Sign up for GitHub

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

Add prettier.config setting #433

Closed
wants to merge 4 commits into from
Closed

Add prettier.config setting #433

wants to merge 4 commits into from

Conversation

gakada
Copy link

@gakada gakada commented May 1, 2018 •
edited
Loading

For #254.

CiGit
CiGit requested changes May 2, 2018
View reviewed changes
Copy link
Member

@CiGit CiGit left a comment

Choose a reason for hiding this comment

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

I've left some comments with change I'd like to see.

Could you also update the changelog file ?

README.md Outdated
@@ -110,6 +110,9 @@ Require a 'prettierconfig' to format
Supply the path to an ignore file such as `.gitignore` or `.prettierignore`.
Files which match will not be formatted. Set to `null` to not read ignore files. Restart required.

#### prettier.configPath (default: not used)
Copy link
Member

Choose a reason for hiding this comment

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

Could we name this config instead of configPath.
prettier (cli) names this option --config

README.md Outdated
@@ -110,6 +110,9 @@ Require a 'prettierconfig' to format
Supply the path to an ignore file such as `.gitignore` or `.prettierignore`.
Files which match will not be formatted. Set to `null` to not read ignore files. Restart required.

#### prettier.configPath (default: not used)
Supply the path to a prettier config file such as `.prettierrc`. The path should be relative to the workspace folder.
Copy link
Member

Choose a reason for hiding this comment

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

It could also be an absolute path?

src/PrettierEditProvider.ts Outdated
const { config: fileOptions, error } = await resolveConfig(fileName, {
editorconfig: true,
config: workspaceFolder && configPath && join(workspaceFolder.name, configPath)
Copy link
Member

Choose a reason for hiding this comment

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

Missing absolute path handling

This relative to workspaceFolder / absolutePath handling could be extracted to a function. This is already used for prettierignore (others?)

src/configCacheHandler.ts Outdated
@@ -15,6 +16,12 @@ const PRETTIER_CONFIG_FILES = [
'prettier.config.js',
];

const configPath = getConfig().configPath;
Copy link
Member

Choose a reason for hiding this comment

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

This is only handling User settings. And would need a restart in case the setting (not the file) is changed.

@gakada
Copy link
Author

gakada commented May 10, 2018

Did some changes. getAbsolutePath from utils is used for path handling for both ignore and config files. For configCacheHandler

const prettierConfigPath = getConfig().config;

if (prettierConfigPath) {
    PRETTIER_CONFIG_FILES.push(basename(prettierConfigPath));
}

the intention is to watch for prettier config files, for setting files restart is required, same as for ignorePath setting (noted in readme).

@gakada gakada changed the title Add prettier.configPath setting Add prettier.config setting May 10, 2018
@CiGit
Copy link
Member

CiGit commented Jun 1, 2018

Sorry for the delay.

Quickly went through (I will need a little more time to review), seems fine.
Last question (hopefully), there is a check hasPrettierConfig, shouldn't it return true if a config path is given and that config file exists ?

@gakada
Copy link
Author

gakada commented Jun 5, 2018

Right, so for files like .prettierrc default configuration is (mostly) ignored (so it is bundledPrettier configuration). Added a commit for the same behavior when config is set.

Another thing, if config is set but doesn't exist, regular config files like .prettierrc are ignored, but that is also how prettier.resolveConfig is.

@tvald
Copy link

tvald commented Jun 17, 2018

I'd love to see this feature merged.
@gakada any chance to fix the conflict? (I'm happy to fix and make a new PR)
@CiGit was your question addressed above?

@tvald tvald mentioned this pull request Jun 17, 2018
Open
CiGit
CiGit requested changes Aug 6, 2018
View reviewed changes
Copy link
Member

@CiGit CiGit 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 your changes, still some changes to handle every possible config files.
It's a resource scope setting :-)

src/configCacheHandler.ts
@@ -15,6 +17,12 @@ const PRETTIER_CONFIG_FILES = [
'prettier.config.js',
];

const prettierConfigPath = getConfig().config;
Copy link
Member

Choose a reason for hiding this comment

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

There may be multiple config files, one per workspace folder.
This is just reading one.

To do it correctly, we would need to set up file change hooks for every files. And remove them when needed.

When a workspace folder is added, check the config and register a hook for it if needed
When a workspace folder is removed, remove the file watcher if it exists

src/configCacheHandler.ts
const prettierConfigPath = getConfig().config;

if (prettierConfigPath) {
PRETTIER_CONFIG_FILES.push(basename(prettierConfigPath));
Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't be in this array, see previous comment.

src/utils.ts
* Get absolute path for relative or absolute filePath.
* Relative to uri's workspace folder in relative case.
*/
export function getAbsolutePath(uri: Uri, filePath: string): string | undefined {
Copy link
Member

Choose a reason for hiding this comment

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

👍

@ntotten
Copy link
Member

ntotten commented Aug 23, 2019

Closing as this is very outdated.

@ntotten ntotten closed this Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CiGit CiGit CiGit requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@gakada @CiGit @tvald @ntotten