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

Add PositionEncodingKind and friends from 3.17 #248

Merged

Conversation

DirectXMan12
Copy link
Contributor

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
Copy link
Contributor Author

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 DirectXMan12 force-pushed the feature/3.17-offset-encodings branch from c5af9b2 to 1d112a7 Compare September 7, 2022 04:59
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 DirectXMan12 force-pushed the feature/3.17-offset-encodings branch from 1d112a7 to 794c22d Compare September 7, 2022 05:18
@DirectXMan12
Copy link
Contributor Author

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

@Marwes Marwes merged commit dbf23ed into gluon-lang:master Oct 21, 2022
@Marwes
Copy link
Member

Marwes commented Oct 21, 2022

Released as 0.93.2

@dsherret
Copy link

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
Copy link
Member

Marwes commented Oct 28, 2022

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
Copy link

dsherret commented Oct 28, 2022

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
Copy link
Member

Marwes commented Oct 28, 2022

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 🤷 .

@Veykril
Copy link

Veykril commented Oct 28, 2022

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
Copy link

That makes sense. Thanks!

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.

5 participants