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

Feature/pass filename to prettier #620

Merged
merged 12 commits into from
Jun 23, 2020

Conversation

simschla
Copy link
Contributor

Here is my take for fixing #448 - pass the filename into prettier to let it choose the correct formatter on its own.

Based on the name of the currently formatted file, prettier is now able to use different parsers on a filecollection containing mixing resources such as js/html/css and the like by peeking at the actual filename.
Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

Awesome! File is meant to really be optional and nullable, so some changes are needed for that, but otherwise looks good.

and create a separate instance dedicated to support filenames
(and since we already handle the "empty" filename correctly, there is nothing to done)
@@ -38,7 +38,8 @@ void install() {
}

Process start() {
return npm("start");
// adding --scripts-prepend-node-path=true due to https://github.com/diffplug/spotless/issues/619#issuecomment-648018679
return npm("start", "--scripts-prepend-node-path=true");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a reaction to the discussion in #619 - make sure we always use the node binary associated with the npm binary we use.

simschla and others added 4 commits June 23, 2020 19:19
…epends on:

- the content that gets passed in
- the filename
- the content which is expected

And that's it - there's no filename in and different filename out.  So there should only be 3 arguments, not 4.  Also brought format to be consistent with StepHarness.
@nedtwigg
Copy link
Member

I made some tweaks to StepHarnessWithFile, I think it had too many arguments before. I believe the only substantive change that I made is fb0390d. This LGTM once CI finished, feel free to merge, or if you want to make further changes feel free :)

@simschla
Copy link
Contributor Author

I made some tweaks to StepHarnessWithFile, I think it had too many arguments before. I believe the only substantive change that I made is fb0390d.

Awesome, thank you for that - the changes make sense and reducing the degrees of freedom in StepHarnessWithFile makes sense. I’ll go ahead and merge.

@simschla simschla merged commit db915dd into diffplug:main Jun 23, 2020
@simschla simschla deleted the feature/pass-filename-to-prettier branch June 23, 2020 19:58
@nedtwigg
Copy link
Member

nedtwigg commented Jul 2, 2020

Published in maven 2.0.0 and gradle 4.5.0

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