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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Omit severity for removing the warning from prettier #10

Merged
merged 2 commits into from Nov 11, 2018

Conversation

Schniz
Copy link
Contributor

@Schniz Schniz commented Nov 8, 2018

Hey! Thanks for this package 馃樃

Right now, all options are passed automatically right from the stylelint-prettier rule into prettier itself.
That causes a warning if you pass something that is related to stylelint only, like severity, So the following line actually raises a warning:

{
  "rules": {
    "prettier/prettier": [true, { "severity": "warning" }]
  }
}

This warning can be avoided, so this PR fixes it by omitting severity. Figuring out which keys should be allowed may be problematic if Prettier will change its API, but I guess that severity is something that won't be there anyway because it's not a linter.

What do you think?

馃樃

@BPScott
Copy link
Member

BPScott commented Nov 9, 2018

Thanks for spotting this.

In retrospect, What I really should have is done is have the prettierrc config as a key in those secondary options, rather than treating the whole block as options to pass to prettier. As it currently stands I've backed myself into a corner and made it impossible to add additional options, along with causing the headache having to extract out the built-in options you've spotted.
changing

Changing the config like below is easily fixable but will review a major version bump as it'll be a breaking change.

Current (problematic as it requires extracting severity and message from the prettier options):

"prettier/prettier": [true, { 
  "severity": "warning",
  "singleQuotes": true
}]

Future (better as you don't need to extract their builtins):

"prettier/prettier": [true, {
  "severity": "warning",
  prettierOptions: {"singleQuotes": true}
}]

In the mean time lets go with your solution of extracting out those common options. I've got a few requested changes:

@Schniz
Copy link
Contributor Author

Schniz commented Nov 11, 2018

Hi! thanks for the reply 馃樃

In retrospect, What I really should have is done is have the prettierrc config as a key in those secondary options, rather than treating the whole block as options to pass to prettier. As it currently stands I've backed myself into a corner and made it impossible to add additional options, along with causing the headache having to extract out the built-in options you've spotted.

You're right. Sounds better 馃憤
I guess that I'd rather just fix this small issue so it won't bother my build 馃樃
I can later on add a PR that makes this change too - if you want

... message is also a built-in option ...

Right. I'll fix that too 馃樃

  • Can you add a test case showing that you get no warnings if you specify a custom severity and message.

Sure, my bad that it wasn't there in the first place 馃憤

... we can do this in a terser way by using rest operators ...

Yup. That was the first thing I tried, but unfortunately the engine.node key in package.json suggests Node 6 and above, and unfortunately, Node 6 doesn't support object spread natively:

"engines": {
"node": ">=6"
},

This can be changed as well, but probably should be considered as a breaking change too

},
});

return linted.then(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no native async await in Node 6 as well 馃

@BPScott BPScott merged commit 8a874c2 into prettier:master Nov 11, 2018
@BPScott
Copy link
Member

BPScott commented Nov 11, 2018

Thanks again @Schniz. I've published this as v1.0.4.

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

2 participants