nrwl / precise-commits

:sparkles: Painlessly apply Prettier by only formatting lines you have modified anyway!
MIT License
471 stars 20 forks source link

Not removing empty lines #8

Closed rafaelchiti closed 6 years ago

rafaelchiti commented 6 years ago

Hey hey 👋, first of all thanks for sharing this lib, looking awesome. I got a question, imagine the following flow:

I got a random js file as follows:

import lib from "lib";

lib.something();

Then I add changes to it (will represent the changes with a + at the beginning of the line)

import lib from "lib";

+
+
+ const a = b => console.log(b);
+
+
+
lib.something();

Then I run npm run precommit, my file changes to (considering my settings of prettier):

import lib from "lib";

const a = (b) => console.log(b);

lib.something();

It did apply formatting (added the parens to the arrow function) but didn't remove all the empty lines. I wonder if we can ever work around this, I imagine you are running prettier on a given 'fragment of code' so it would never remove those empty lines, prettier would only remove them when applying formatting to the whole file. Any ideas? I don't want people to start introducing random amount of new empty lines on commits.

Thanks!!

JamesHenry commented 6 years ago

I'm inclined to say that this is more of a question for prettier itself, and how it approaches formatting a given range.

Please could you try running prettier directly on your example, but using the rangeStart and rangeEnd options for prettier to only format your new code (and newlines) range?

If the result differs, it is definitely something to look into for precise-commits, otherwise I think we'll need to take the discussion over to the prettier repo.

rafaelchiti commented 6 years ago

Thanks for the quick reply!, not sure how rangeStart works, the only docs I found are here https://prettier.io/docs/en/api.html#prettierformatwithcursorsource-options but says nothing about rangeStart. I gave it a try in the client with --range-start and --range-end, it does not work as I would expect (it keeps the empty lines), but not sure I'm using the rangeStart options properly (it does format the line of code though).

On the other hand if I make a selection in the editor (VSCode), including the empty lines above and below the line of code, it formats the code and removes the empty lines (as I would expect). Wondering what is the difference between what the editor uses and the prettier client as I'm testing it out.

JamesHenry commented 6 years ago

Yeah I don't know off the top of my head how the editor integration is configured. The docs for rangeStart and rangeEnd are with the other options just FYI: https://prettier.io/docs/en/options.html#range

I will try and look into this later on this week, but it sounds like maybe you have shown it is just how prettier handles this particular case?

rafaelchiti commented 6 years ago

well I'm a bit uncertain on what the difference between rangeStart through the client vs the editor format selection, certainly they do different things, would be great to mimic what the editor does, will take a look at the VSCode and see if there is any info available.

rafaelchiti commented 6 years ago

mmm now that I think about it what the plugin does when you select text is most likely to create a buffer (with it) and apply prettier as if that was the entire file, that is why is behaving like prettier does when formatting the whole file (but only on your selection). Will dig around in prettier repo and check whether rangeStart / rangeEnd is supposed to work this way or not.

I think is not an issue for your repo, if you agree feel free to close it!

Thanks!!

JamesHenry commented 6 years ago

@rafaelchiti I added the capability to use range formatting within the prettier playground so I can now reproduce your example there:

https://prettier.io/playground/#N4Igxg9gdgLgprEAuEBLAtgBwgJxgAgBtUAjfAMxwnXwB0RiT6BuWqNjqSKAZwIEN8AXnxkhAPnzceEQnAB0hCAHMAFCQCUrdjs6N5M9HBgALVFDVaQAGhARMMVNB7JQ-HFQDuABXcIXKPwAbhCoACY2ICQ4-GAA1sYAypix5srIMDgArnC25jxweN4xyuj8yOT8hAW2AFY8AB4AQjHxSfxGADLmcBVVNSApOAU4yFH8JACeSlCRmDjmMADq4abIABwADLbzEAVLMZhj83AjQb22OHAAjlmoV8X8peVIldW5IAXoqBnZHzxpOQARSyEHgfXethgExWYTWSAATFCYqhiBYAMLUMpjKDQC4gLIFAAqEwCbwGMQscES0LwyARSJAlOUcAAolAIkgAGybAC+vKAA

It looks like the whitespace is ignored by prettier.

This is not something we easily change in precise-commits, as this is just how prettier handles this particular scenario. Please feel free to open an issue on the prettier repo with the reproduction if you feel it is worth discussing. I will close this for now, thanks!