-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
cli: don't split formatter:output on Windows drive letters (#953) #954
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for fixing this. I'll merge this in once the comment is addressed
.prettierrc.yml
Outdated
@@ -0,0 +1,3 @@ | |||
trailingComma: none | |||
singleQuote: true | |||
semi: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this file. This configuration is in .eslintrc.yml
. And you can pretty-ify code with eslint --fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I get that it’s in the eslint config. That feels like a workaround because prettier didn’t support config files for a while. The reason I like it outside of eslint is because I have my editor set up to run prettier on save if it’s available locally in the project so it’s conveinent to have those preferences declared in a format that’s native to the tool. I contribute here so infrequently that it’s not a big deal to me either way. Prettier will also read config from package.json
if it’s the extra file you don’t like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want it to live in one place. I'm fine removing it from .eslintrc.yml
if having this config file is enough. Either way though, that is an unrelated change to this PR and thus I think should be removed (and be in its own PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although if living in one place is the goal then eslint is probably the place to put it since that enforces the style. Looks like neoformat supports prettier-eslint so I'll get that configured next time I'm in here.
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
That regex feels hacky to me so if there is a way to do this with a single
split
without checking the number of split parts I'd be more than happy to use it. It's still splitting on a:
, but not if it is preceded by a capital letter and followed by a/
(at least that was my intention). I looked at this old pr, but couldn't tell if that would work in this situation. Regex hurts my head.