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 option to skip loading prettierrc #83

Merged
merged 2 commits into from Feb 2, 2018

Conversation

eliperelman
Copy link
Contributor

Hello! This PR adds an option to allow you to skip loading the prettierrc file. I have a use case for this in some external tooling and it would really come in handy.

@not-an-aardvark
Copy link
Member

Thanks for the PR. I'm fine with adding something like this, but I think it could be confusing to include it in the same object as the other prettier options, since this isn't an option that gets passed to prettier. Can you make this a second argument instead? For example, it could get configured as prettier/prettier: [error, {}, { usePrettierrc: false }]

@eliperelman
Copy link
Contributor Author

@not-an-aardvark absolutely, I like that route waaay better! Will revise.

@eliperelman
Copy link
Contributor Author

@not-an-aardvark updated!

@eliperelman
Copy link
Contributor Author

For posterity, this option was inspired by ESLint CLIEngine's useEslintRc option.

https://eslint.org/docs/developer-guide/nodejs-api#cliengine

Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

Thanks, I just have a few minor comments.

README.md Outdated
}]
```

- `usePrettierRc`: Enables loading of the Prettier configuration file, set to `false` to disable. May be useful if you are using multiple tools that conflict with each other, or do not wish to mix your ESLint settings with your Prettier configuration.
Copy link
Member

Choose a reason for hiding this comment

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

Minor nitpick: Can we rename the option to usePrettierrc (lowercase r)? That would also be consistent with the useEslintrc option that this is based on.

README.md Outdated
}]
```

- `usePrettierRc`: Enables loading of the Prettier configuration file, set to `false` to disable. May be useful if you are using multiple tools that conflict with each other, or do not wish to mix your ESLint settings with your Prettier configuration.
Copy link
Member

Choose a reason for hiding this comment

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

set to false to disable

Nitpick: Can you replace this text with "(default: true)"? The double negative in "set to false to disable" makes it a bit confusing to read.

type: 'object',
properties: {
pragma: { type: 'string', pattern: '^@\\w+$' },
usePrettierRc: { type: 'boolean' }
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test for the new option? (It would probably be similar to the existing prettierrc tests, except that the test would be asserting that the options in the file do not get applied.)

@eliperelman
Copy link
Contributor Author

@not-an-aardvark ok, I think all your requests have been made! I also added a test to ensure you could still use pragma from the options object as well.

@azz
Copy link
Member

azz commented Jan 31, 2018

If this is a breaking change (moving pragma from first object to second), I'd recommend removing it entirely as it is now built-in to Prettier.

Perhaps a more ergonomic API would be to do:

"prettier/prettier": ["error"], // use config file
"prettier/prettier": ["error", {}], // do not use config file
"prettier/prettier": ["error", { "singleQuote": true }], // do not use config file

@not-an-aardvark
Copy link
Member

I think this is backwards-compatible -- pragma was already in the second slot. (I agree that it should probably be removed in the next major release.)

@eliperelman
Copy link
Contributor Author

This is not a breaking change, it's a minor bump. Using a string pragma is still supported in the second slot.

@azz
Copy link
Member

azz commented Jan 31, 2018

Yeah I misinterpreted the change, ignore the first part of my comment.

@not-an-aardvark
Copy link
Member

If I'm understanding correctly, the second part of the comment would be a breaking change, right? Maybe we should merge this as-is and then implement the second part of the comment in a major release (at some indefinite point in the future if a major release is necessary).

Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

Copy link
Member

@azz azz left a comment

Choose a reason for hiding this comment

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

lgtm. I do think #83 (comment) is worth doing in a future release.

@not-an-aardvark not-an-aardvark merged commit 1f50134 into prettier:master Feb 2, 2018
@not-an-aardvark
Copy link
Member

Thanks for contributing!

@eliperelman
Copy link
Contributor Author

My pleasure, thank you for accepting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants