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

Support prettierignore and custom processors #111

Merged
merged 3 commits into from
Sep 26, 2018
Merged

Support prettierignore and custom processors #111

merged 3 commits into from
Sep 26, 2018

Conversation

BPScott
Copy link
Member

@BPScott BPScott commented Sep 26, 2018

Fixing two bugs as one provides the scaffolding of the fix for the other.


Ignore files in .prettierignore

Fixes #88


Force the babylon parser when parsing not-js files

Forcing the parser stops errors that are caused by trying to run a JS
fragment through the graphql / markdown parsers.

The 'html' parser has not yet been released yet, but will be in Prettier
1.15.

This uses a block list over an allow list because I expect the list of
"file types with a prettier parser that could contain javascript fragments"
will grow at a slower pace than "file types with a prettier parser that
are variations of the javascript language".

Fixes #98, Fixes #81

Forcing the parser stops errors that are caused by trying to run a JS
fragment through the graphql / markdown parsers.

The 'html' parser has not yet been released yet, but will be in Prettier
1.15.

This uses a block list over an allow list because I expect the list of
"file types with a prettier parser that could contain javascript fragments"
will grow at a slower pace than "file types with a prettier parser that
are variations of the javascript language".

Fixes #98, Fixes #81
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 for the PR! The general approach looks good to me -- I left a few minor suggestions for improvement.

@@ -65,6 +65,11 @@ ruleTester.run('prettier', rule, {
code: `var foo = {bar: 0};\n`,
filename: getPrettierRcJsFilename('bracket-spacing'),
options: [{ bracketSpacing: false }, { usePrettierrc: false }]
},
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 parser blacklist behavior? We don't need to actually include processors, but maybe one way to do it would be to put JavaScript code in a file with the graphql extension, and assert that no parsing errors are produced (since the text should be parsed as JavaScript).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. I tried doing this with including a plugin, but I couldn't get the plugin to be applied within a RuleTester. Forcing the extension is a good middle ground

// This is added to the options first, so that
// prettierRcOptions and eslintPrettierOptions can still override
// the parser.
const parserBlocklist = [null, 'graphql', 'markdown', 'html'];
Copy link
Member

Choose a reason for hiding this comment

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

This list seems like it might plausibly need to expand in the future. The detailed comment above it with background is definitely appreciated, but since the situation is fairly complex and difficult to comprehend, is there a simple description we can add for what the list should contain for future reference?

For example, maybe we could add a comment stating that:

This list should contain the list of prettier parser names for file types where:

  • Prettier supports parsing the file type
  • There is an ESLint processor that extracts JavaScript snippets from the file type.

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.

LGTM, thanks!

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