gluon-lang / lsp-types

Types for communicating with a language server
MIT License
328 stars 86 forks source link

Add PositionEncodingKind and friends from 3.17 #248

Closed DirectXMan12 closed 2 years ago

DirectXMan12 commented 2 years ago

3.17 introduces PositionEncodingKind to indicate how positions are encoded (utf-8, utf-16, or utf-32). There are also corresponding fields in the client (positionEncodings) and server (positionEncoding) capabilities to negotiate which to use.

DirectXMan12 commented 2 years ago

Of note: I introduced these marked as feature = "proposed" because they're 3.17, but it seems like 3.17 features are no long in the proposed phase (the changelog seems to say it's been released since May), so I'm not sure how y'all want to handle that

DirectXMan12 commented 2 years ago

hey @Marwes hate to do the ping here, but is there anything else this PR needs?

Marwes commented 2 years ago

Released as 0.93.2

dsherret commented 2 years ago

For next time, it probably should have been 0.94 because there's new properties. Cargo's lockfiles are almost useless for publishing unfortunately :(

Marwes commented 2 years ago

This only added items hidden under the proposed feature which this repo explicitly exempts from semver https://github.com/gluon-lang/lsp-types#lsp-types (I don't want to be forced to do a major release every time a proposed feature is added or changed as those are by definition, unstable).

dsherret commented 2 years ago

I see, I'd still recommend just bumping the minor (0.94) and you don't need to bump the major as it will only eagerly pull in patch releases.

The problem with doing this in patch releases is it breaks cargo publishes downstream and creates some headaches. For example, we depend on another crate which depends on this crate and it doesn't pin the dependency on this crate. So even though our cargo build uses a lockfile, the cargo publish will pull in the latest patch release and things break. Or someone could publish a binary to crates.io, it works ok, this crate publishes a patch, then someone does cargo install without --locked and now that doesn't work anymore either.

Marwes commented 2 years ago

Bumping the minor version would force everyone that is not using any proposed features to update their Cargo.toml every time something changes under the proposed even if they would otherwise be covered under a patch/non-breaking update. This seems worse to me than making users who use unstable features to lock down their dependencies harder (add =0.93.0 constraint in their Cargo.toml).

If I weren't using this scheme of breaking changes under proposed I would just not merge any proposed lsp features until they were stable :shrug: .

Veykril commented 2 years ago

Pinning if you are using proposed sounds like the correct choice here I agree. I would say its the crate's fault if it depends on proposed without pinning the version (r-a did this until 0.93.2 accidentally, we are now pinned as well).

dsherret commented 2 years ago

That makes sense. Thanks!