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

Added chained "pipe" function formatting tests #6818

Merged
merged 4 commits into from Jun 3, 2020

Conversation

brodybits
Copy link
Contributor

In response to #6469 (comment) by @lydell, here are a couple of tests that show how Prettier handles the formatting of "pipe" functions and chained "pipe" functions:

  • test of input that is clean and correct according to Prettier 1.18.2, unfortunately seems to be made hard to read in the master branch
  • another test with some comments that I added, which will keep things on separate lines as I think is what we want.

@lydell please feel free to correct and improve this explanation as you feel would be justified.

I would like to make the following points:

  • I do think that the formatting of the input with no comments added is an unfortunate change in this case.
  • I have discovered that strategic placement of comments can sometimes help give the user cleaner formatting, in addition to some descriptive insights. I think this is something that should be formally documented and would be happy to work on this (someday).
  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to the CHANGELOG.unreleased.md file following the template.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@lydell
Copy link
Member

lydell commented Nov 5, 2019

Adding a // comment is a good old trick to force multiline formatting. A // comment requires a newline after it (since it comments to the end of the line) – that’s why Prettier is forced to switch to multiline formatting if a // comment is encountered. (Unless Prettier moves the comment somewhere else, which it does sometimes.)

@brainkim
Copy link
Contributor

brainkim commented Nov 5, 2019

Like I said in the original PR, I’m here to help! Ping me if you come up with any ideas/need to understand some code related to call expression splitting. I don’t think the above issue is actionable in its current form but I’m very willing to accommodate requests for new heuristics. I don’t want to be the one to hold up 1.19.

@lydell
Copy link
Member

lydell commented Nov 5, 2019

I agree with @brainkim. What’s your intention with this PR, @brodybits?

@brodybits
Copy link
Contributor Author

Like I said in the original PR

in #6469 (comment) (I am adding the explicit link for the sake of some less experienced Prettier developers such as myself)

What’s your intention with this PR, @brodybits?

I have a feeling that the formatting behavior in this case should be explicitly tested in the test suite, visible if it changes in a minor release. I think the change from 1.18.2 to master that is demonstrated here should ideally be avoided in a minor release.

I also wanted to demonstrate the trick of using comments to get cleaner formatting in some cases, since it was not obvious to me at first. I think it should be documented at some point.

I am fine if you guys want to close this PR.

@lydell
Copy link
Member

lydell commented Nov 5, 2019

These are good test cases, so I think we should merge.

@brodybits brodybits marked this pull request as ready for review November 5, 2019 18:48
@thorn0
Copy link
Member

thorn0 commented Nov 5, 2019

I think these tests should be placed in tests/functional_composition. No need to create a new directory.

@brodybits brodybits changed the title [RFC ...] chained "pipe" function formatting tests [WIP] chained "pipe" function formatting tests Nov 6, 2019
Christopher J. Brody and others added 2 commits November 5, 2019 19:42
which was correct according to prettier@1.18.2

Co-authored-by: Simon Lydell <simon.lydell@gmail.com>
Co-authored-by: Christopher J. Brody <chris@brody.consulting>
in order to avoid unwanted reformatting

Co-authored-by: Simon Lydell <simon.lydell@gmail.com>
Co-authored-by: Christopher J. Brody <chris@brody.consulting>
@brodybits brodybits force-pushed the pipe-function-formatting-tests branch from 3bd9d10 to cb4494d Compare November 6, 2019 00:46
@brodybits brodybits changed the title [WIP] chained "pipe" function formatting tests [RFC] chained "pipe" function formatting tests Nov 6, 2019
@brodybits
Copy link
Contributor Author

I just moved the new tests into tests/functional_composition and did a clean rebase. The one thing now is that the new test scripts have slightly incconsistent file names, using dash instead of underscore. I personally prefer dash over underscore in file names whenever possible, would be happy to rename if needed.

@thorn0 thorn0 added the type:tests Issues about tests that are not correct, should be added, or similar label Feb 25, 2020
# Conflicts:
#	tests/functional_composition/__snapshots__/jsfmt.spec.js.snap
#	tests/js/functional-composition/pipe-function-calls-with-comments.js
#	tests/js/functional-composition/pipe-function-calls.js
@fisker fisker changed the title [RFC] chained "pipe" function formatting tests Added chained "pipe" function formatting tests Jun 3, 2020
Copy link
Member

@fisker fisker left a comment

Choose a reason for hiding this comment

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

I did a rebase, should good to merge.

@fisker fisker merged commit 7659a86 into prettier:master Jun 3, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type:tests Issues about tests that are not correct, should be added, or similar
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants