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

Remove claim, untrue since over 5 years ago, that cursorOffset is incompatible with rangeStart/rangeEnd #15750

Conversation

ExplodingCabbage
Copy link
Contributor

@ExplodingCabbage ExplodingCabbage commented Dec 4, 2023

Description

In May 2017, @josephfrazier added the cursorOffset option. This initial implementation of cursorOffset was incompatible with rangeStart/rangeEnd, and so Joseph added lines to the docs about cursorOffset and rangeStart/rangeEnd saying:

This option ... cannot be used with rangeStart and rangeEnd.

and

This option cannot be used with cursorOffset.

This was all true and reasonable at the time, but the next year, in April 2018, the actual incompatibility was fixed by @ds300 in #4397. But I guess that neither he nor anyone who reviewed his PR noticed the documentation about the incompatibility still exists, because it wasn't removed at the time and still exists to this day, even though David's fix made it obsolete.

I suggest we finally remove those false statements, 5 years later. This PR does that. :)

Checklist

  • I’ve added tests to confirm my change works. - No reason to, though I've updated existing snapshots as needed.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory). - N/A
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@ExplodingCabbage ExplodingCabbage marked this pull request as ready for review December 4, 2023 18:52
@thorn0
Copy link
Member

thorn0 commented Dec 4, 2023

Thanks! Could you also change the types (src/index.d.ts) accordingly?

@ExplodingCabbage ExplodingCabbage force-pushed the remove-false-claim-about-incompatible-options branch from 5d20c84 to 43c6944 Compare December 5, 2023 10:37
@ExplodingCabbage
Copy link
Contributor Author

Could you also change the types (src/index.d.ts) accordingly?

Ah, interesting - I hadn't looked at the types and hadn't noticed that CursorOptions used properties of type never to forbid rangeStart or rangeEnd from being set (or at least to purport to - clearly it still actually worked!). I've removed those properties.

Hmm... but wait, there's nothing special any more about cursorOffset to justify there being a dedicated CursorOptions type at all, is there? So it probably makes sense to eliminate the type entirely, in favour of simply adding a cursorOffset property to RequiredOptions akin to all the other properties on there. So I've gone ahead and done that too.

(One conceivable objection I see to this is that it might break compatibility with packages that are currently importing CursorOptions. I defer to you guys as Prettier's maintainers on whether it is likely that such packages exist and, if so, whether breaking compatibility with them is acceptable if so; I have no idea on either point.)

Tangential aside: what is the sense in which RequiredOptions are "required"? That naming is puzzling to me and I hesitated a bit about adding cursorOffset to the "required" options because passing a cursorOffset when calling Prettier is of course entirely optional before realising that of course the same is true of literally every option and so "required" must mean something else. It might be worth chucking a one- or two-sentence doc comment above the type explaining its meaning!

@fisker
Copy link
Member

fisker commented Dec 5, 2023

In format() and check(), cursorOffset is useless, see #12803

@ExplodingCabbage
Copy link
Contributor Author

In format() and check(), cursorOffset is useless, see #12803

... and therefore you'd like to keep a separate CursorOptions type, simply to communicate via the type system that some functions that take options respect cursorOffset and others don't? Okay, I'll revert 43c6944 then.

(If I'm not inferring your wishes correctly, please go ahead and just edit the PR as you see fit - I've got it set to allow edits by maintainers!)

…b.com:ExplodingCabbage/prettier into remove-false-claim-about-incompatible-options
@thorn0
Copy link
Member

thorn0 commented Dec 5, 2023

I can't see the type changes in the resulting diff

@ExplodingCabbage
Copy link
Contributor Author

@thorn0 the type changes that I haven't reverted (in response to @fisker's feedback above) show up for me:

image

@thorn0
Copy link
Member

thorn0 commented Dec 5, 2023

@ExplodingCabbage Oops, somehow overlooked it
@fisker What's that CI error? Something known?

@fisker
Copy link
Member

fisker commented Dec 5, 2023

Type test is failing, don't know why. https://github.com/prettier/prettier/tree/main/tests/dts/unit

@thorn0
Copy link
Member

thorn0 commented Dec 5, 2023

Figured it out #15766
@ExplodingCabbage please remove @ts-expect-error in tests/dts/unit/cases/standalone.ts

@ExplodingCabbage
Copy link
Contributor Author

@thorn0 done - and yarn test passes locally now.

@thorn0 thorn0 merged commit 1e30f66 into prettier:main Dec 8, 2023
28 checks passed
@josephfrazier
Copy link
Collaborator

Thanks for catching and fixing this, @ExplodingCabbage!

@ExplodingCabbage ExplodingCabbage deleted the remove-false-claim-about-incompatible-options branch December 11, 2023 09:40
medikoo pushed a commit to medikoo/prettier-elastic that referenced this pull request Feb 16, 2024
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

4 participants