hosseinmd / prettier-plugin-jsdoc

A Prettier plugin to format JSDoc comments.
MIT License
232 stars 28 forks source link

feat: add option for wrapping strategy #172

Closed vincerubinetti closed 2 years ago

vincerubinetti commented 2 years ago

related: #65 #168

This is likely to be made irrelevant by @egilll's upcoming PR, which also should hopefully contain a "normal" mode like I am wanting:

https://github.com/egilll/prettier-plugin-jsdoc-balanced

https://github.com/egilll/prettier-plugin-jsdoc-balanced/blob/master/src/index.ts#L29-L32

vincerubinetti commented 2 years ago

I haven't added tests yet. If you change the default from smart to simple, tests will (rightly) fail.

I'm not sure how far I should pursue this, because @egilll 's PR seems to be way more comprehensive than mine.

egilll commented 2 years ago

In my view this shouldn't be an option, the last lines should simply never have any extra width.

vincerubinetti commented 2 years ago

I agree with you, but apparently other people requested it.

Regardless, the approach of making this a string option, like "some-strategy" | "some-other-strategy", is the way to go, just like what you're doing in your PR.

hosseinmd commented 2 years ago

In my Opinion, we should ignore current strategy and add fair line wrapper as an option in future.

hosseinmd commented 2 years ago

However, This PR LGTM because there is no break change.

Please add a test with new option.

vincerubinetti commented 2 years ago

In my Opinion, we should ignore current strategy and add fair line wrapper as an option in future.

Sorry, are you saying we should basically just get rid of extraLastLineWidth, essentially reverting back to before this discussion changed things? If so, this PR could be a lot simpler.

However, personally, fixing this issue is not a dire emergency. I could wait until @egilll makes their PR.

hosseinmd commented 2 years ago

Sorry, are you saying we should basically just get rid of extraLastLineWidth, essentially reverting back to before this discussion changed things? If so, this PR could be a lot simpler.

Yes, I think simple wrapper could be default strategy and fair be second strategy.

ZerdoX-x commented 2 years ago

Сan't wait for this to show up. Thank you!

vincerubinetti commented 2 years ago

Okay, I've made it into just a single option for now. I've renamed the option to match @egilll 's fork. I've removed extraLastLineWidth all together, and updated the test snapshots to match.


Note that I ran into a different issue while testing:

test("numbers and code in description", () => {
  const result1 = subject(`
/**
 * =========================== PressResponder Tutorial =========================== 
`)

  // etc.
})

Running subject(subject()) on this, as you do in this test, yields something like * # =========================== PressResponder Tutorial. My best guess is that this is due to these lines?

For now, I've just removed the extra =s, because I believe this is a separate problem that merits its own issue.

I believe this is a false negative. That is, this test was passing before, because you had extraLastLineWidth, and the number of extra =s you put in the test happened to be less than that.

hosseinmd commented 2 years ago

Looks good to me

hosseinmd commented 2 years ago

This change is a breaking change on how to format code but not on how to use plugin, I think we could move to v0.4.

hosseinmd commented 2 years ago

Released v0.4.1