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

Add a new `trim` command to trim whitespaces in the current line (#4771). #4772

Merged
merged 1 commit into from Oct 6, 2018

Conversation

Projects
None yet
3 participants
@warrenseine
Contributor

warrenseine commented Jun 29, 2018

I reused and factorized the pre-existing code to trim trailing spaces.

Note that this adds no test, and I'm not really confident about that. How would you test this change? I couldn't find any test that explicitly used hardline for instance.
Fixes #4771

@suchipi

Looks good to me but I'm not familiar with the doc printer.

@@ -146,6 +166,10 @@ function fits(next, restCommands, width, options, mustBeFlat) {
case "align":
cmds.push([makeAlign(ind, doc.n, options), mode, doc.contents]);
break;
case "trim":
trim(out);

This comment has been minimized.

@warrenseine

warrenseine Jun 29, 2018

Contributor

Hum, the bot caught a real bug. out is undefined here.

commands.md Outdated
@@ -237,6 +237,13 @@ declare function dedentToRoot(doc: Doc): Doc;
This will dedent the current indentation to the root marked by `markAsRoot`.
### trim
```ts
declare var literalline: Doc;

This comment has been minimized.

@ikatyang
@@ -244,6 +266,10 @@ function printDocToString(doc, options) {
case "align":
cmds.push([makeAlign(ind, doc.n, options), mode, doc.contents]);
break;
case "trim":
trim(out);

This comment has been minimized.

@ikatyang

ikatyang Jun 30, 2018

Member

I believe we'll have to update the pos (in printDocToString) and the width (in fits) so that the line length can be calculated correctly, we didn't need to do so previously since it's at the end of the line but it's not the case now.

And we should add some tests for it, though it seems we didn't have tests for it from the beginning.

I'm not too familiar with the algorithm here so I might miss something.

This comment has been minimized.

@warrenseine

warrenseine Sep 25, 2018

Contributor

I've rebased the commit and fixed the trivial error.

I've trying to find a way to test this new trim command. All the tests appear to be integration tests (run Prettier on a file and compare snapshot), not unit tests (typical assertion-based unit tests), which means I can't really test the command because it's not used by any of the official formatters (only prettier-csharp). Any idea?

This comment has been minimized.

@ikatyang

ikatyang Sep 26, 2018

Member

You could add a test file for doc in tests_integration/__tests__ (see tests_integration/__tests__/load-toml.js for example), though it's not ideal but we could polish our tests later in other PRs.

This comment has been minimized.

@warrenseine

warrenseine Sep 27, 2018

Contributor

Thank you. It helped. I’ve pushed a couple of tests, what do you think of them?

I will add code and tests to ensure the correct character count in kept for a trimmed line if you think this is ok.

out[out.length - 1].match(/^[^\S\n]*$/)
) {
trimCount += out[out.length - 1].length;
out.pop();

This comment has been minimized.

@ikatyang

ikatyang Sep 30, 2018

Member

trimCount += out.pop().length

Show resolved Hide resolved src/doc/doc-printer.js
@@ -0,0 +1,62 @@
"use strict";

This comment has been minimized.

@ikatyang

ikatyang Sep 30, 2018

Member

Could you rename this file to doc-trim.js? so that it'd be clearer for what's this file used for.

// bundled parser (only third-party plugins).
describe("trim", () => {
test("trims the current line", () => {

This comment has been minimized.

@ikatyang

ikatyang Sep 30, 2018

Member

test.each() with .toEqual() should be better as the output won't be changed.

@warrenseine

This comment has been minimized.

Contributor

warrenseine commented Oct 1, 2018

The CircleCI Node 4 check won't pass, because my test imports some function from a source file that uses ES6 syntax (doc-printer.js). I will have to require("dist/standalone") from the test directly to avoid really on sources. Are we sure that "dist" will always be built before running tests? I see a test:dist target which seems to indicate the opposite. Can you recommend something else? Should I update doc-printer.js to avoid ES6 syntax just for Node 4 support?

"use strict";
const docPrinter = require("../../src/doc/doc-printer");
const docBuilders = require("../../src/doc/doc-builders");

This comment has been minimized.

@ikatyang

ikatyang Oct 1, 2018

Member
const prettier = require("prettier/local");
const docPrinter = prettier.doc.printer;
const docBuilders = prettier.doc.builders;
expect(result.formatted).toEqual(expected);
});
// test("trims the current line", () => {

This comment has been minimized.

@ikatyang

ikatyang Oct 1, 2018

Member

It seems you forgot to remove them.

This comment has been minimized.

@warrenseine

warrenseine Oct 1, 2018

Contributor

My bad, sorry.

// ignores trimmed characters when fitting the line
group(concat(["hello ", " ", trim, line, "world!"])),
"hello world!"
]

This comment has been minimized.

@ikatyang

ikatyang Oct 1, 2018

Member

Could you add a test for trimming to the root (markAsRoot)?

This comment has been minimized.

@warrenseine

warrenseine Oct 1, 2018

Contributor

As is, the code will not respect markAsRoot, and will trim down to column 0. I confirmed this behavior with a test. I can commit that test, but I suppose your comment was about making the code respect markAsRoot.

Do you think it's worth dealing with this edge case? In which situation would I want to keep an indent of 4 spaces, but still want to trim the remaining spaces after?

This comment has been minimized.

@ikatyang

ikatyang Oct 2, 2018

Member

You should update the docs then, and also add a test to ensure this behavior. I guess we probably don't need to deal with it since there's probably no use case for it.

prettier/commands.md

Lines 240 to 246 in 33bceb0

### trim
```ts
declare var trim: Doc;
```
This will trim any whitespace or tab character on the current line to the root marked by `markAsRoot`.

@ikatyang

This comment has been minimized.

Member

ikatyang commented Oct 1, 2018

And also you need to resolve merge conflicts.

"{",
indent(
concat([
markAsRoot(line),

This comment has been minimized.

@ikatyang

ikatyang Oct 2, 2018

Member

It seems this markAsRoot has no effect, it won't affect anything.

I'd expect something like:

[
  // hardline will insert a newline with current indentation
  concat([indent(markAsRoot(indent(hardline))), "123"]),
  "\n    123"
],
[
  // literalline will insert a newline with root indentation
  concat([indent(markAsRoot(indent(literalline))), "123"]),
  "\n  123"
],
[
  // trims up to the the first column, even if there's indented root.
  concat([indent(markAsRoot(indent(literalline))), trim, "123"]),
  "\n123"
],

This comment has been minimized.

@warrenseine

warrenseine Oct 2, 2018

Contributor

Ok, will do. Thanks for your time :)

This comment has been minimized.

@warrenseine

warrenseine Oct 3, 2018

Contributor

These tests won’t pass as is. Did you mean that you expected them to pass and that markAsRoot seems broken?

Anyway, even if they passed, they should probably be part of another test file, say doc-mark-as-root.js. But in that case, I’m not sure what I should do. Maybe tests for markAsRoot should be added in a separate PR, where we clarify what is expected?

This comment has been minimized.

@ikatyang

ikatyang Oct 3, 2018

Member

These tests won’t pass as is. Did you mean that you expected them to pass and that markAsRoot seems broken?

Weird. The tests I mentioned above did pass on my machine, and it's also my expected output.

Anyway, even if they passed, they should probably be part of another test file, say doc-mark-as-root.js. But in that case, I’m not sure what I should do. Maybe tests for markAsRoot should be added in a separate PR, where we clarify what is expected?

I'm fine with either removing the original "trims up to..." test (since it has no effect) or replacing it with the corresponding one I mentioned above.

This comment has been minimized.

@warrenseine

warrenseine Oct 3, 2018

Contributor

I pushed the tests above along with my changes to see if it breaks anything on a different environment. There was a line ending issue I just fixed, but that doesn't seem to be the problem.

group(concat(["hello ", " ", trim, line, "world!"])),
"hello world!"
]
])("trims correctly", (doc, expected) => {

This comment has been minimized.

@ikatyang

ikatyang Oct 2, 2018

Member

We could use the printf-like parameters provided by jest here to improve our test names:

test.each([
  [
    "test name", // change those comments into test names
    doc,
    expected,
  ]
])("%s", (_, doc, expected) => {
//  ^^    ^
})
 PASS  tests_integration/__tests__/doc-trim.js
    ✓ trims the current line
    ✓ trims existing indentation

instead of

 PASS  tests_integration/__tests__/doc-trim.js
    ✓ trims correctly
    ✓ trims correctly
commands.md Outdated
declare var trim: Doc;
```
This will trim any whitespace or tab character on the current line, without inserting a new line. This is used for preprocessor directives.

This comment has been minimized.

@ikatyang

ikatyang Oct 2, 2018

Member

In this context, "without inserting a new line" seems unnecessary.

"\n123"
]
])("%s", (_, doc, expected) => {
const result = printDocToString(doc, { printWidth: 80 });

This comment has been minimized.

@ikatyang

ikatyang Oct 3, 2018

Member

There's no default value for tabWidth in printDocToString, adding tabWidth: 2 should get it passed.

This comment has been minimized.

@warrenseine

warrenseine Oct 3, 2018

Contributor

That's it. It passes on my machine.

@@ -0,0 +1,64 @@
// "use strict";
// const prettier = require("prettier/local");

This comment has been minimized.

@ikatyang

@ikatyang ikatyang merged commit 7920e6f into prettier:master Oct 6, 2018

9 checks passed

ci/circleci: build_prod Your tests passed on CircleCI!
Details
ci/circleci: checkout_code Your tests passed on CircleCI!
Details
ci/circleci: test_prod_node4 Your tests passed on CircleCI!
Details
ci/circleci: test_prod_node9 Your tests passed on CircleCI!
Details
ci/circleci: test_prod_standalone Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 80%)
Details
codecov/project 96.32% (+<.01%) compared to 2283efb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@ikatyang

This comment has been minimized.

Member

ikatyang commented Oct 6, 2018

Thanks!

@ikatyang ikatyang added this to the 1.15 milestone Oct 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment