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 code to expose the selected version of a requested package #56

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

CraftSpider
Copy link

Makes it so the version of the port/package requested is added to the library struct returned, so consumers can do things based on what version they have. Also a drive-by add of Clone to Library, which is useful for the downstream I'm making this change for (tectonic-typesetting/tectonic#1138).

This is a breaking change due to Library not being marked #[non_exhaustive] and having all public fields.

@pkgw
Copy link
Collaborator

pkgw commented Feb 9, 2024

This could be made non-breaking by having the version field be private and adding an associated method, right? I just recently noticed how many downloads vcpkg has which has made me want to be extra-careful about this kind of stuff ...

@CraftSpider
Copy link
Author

CraftSpider commented Feb 9, 2024

I'm afraid not - since all the fields are public, outside users could be constructing instances of the type directly, or matching fields exhaustively, and any change to its fields at all is breaking.

It would be possible to add a new API that returns a new type or just this info, though that has repetition/code cleanliness issues. An alternative is to release a breaking version (0.3 I think?), and if lots of people still use 0.2 after a bit, backport any important fixes.

EDIT: If you do want to go with a breaking change, I'd add a ZST private field to Library, which should ensure no outside construction or exhaustive matching. Unfortunately the #[non_exhaustive] attribute is from Rust 1.40, so more of an MSRV bump than I suspect is worth it.

@pkgw
Copy link
Collaborator

pkgw commented Feb 10, 2024

Hmm, right. @waych Do you have any thoughts on this?

@waych
Copy link
Collaborator

waych commented Feb 11, 2024

Given this crate is so used by others, I agree care is required in justifying the semantic version bump. If we are going to extend the interface at all, we should consider using the opportunity to clean up the crate and do other semantic breaking things at the same time. I think that means iterating here in github for a 0.3 without a push to crates.io until we've bundled enough semantic breakage to make it worthwhile doing.

A possible shortlist of suggestions that could be bundled include: Deleting the parts marked deprecated, fixing the NotMSVC error codes and spaghetti target code, tightening up the types so that they are more private (mark public fields deprecated?) / more semantically extensible in the future, revisit the MSRV. None of these on their own really justify a semantic update, but together they make more sense.

@pkgw
Copy link
Collaborator

pkgw commented Feb 11, 2024

Yeah, there's definitely a good-sized backlog of maintenance tasks that could benefit from a semver-incompatible bump!
We could also consider bumping to 1.0 — I don't always practice what I preach, but I feel like if a package is sufficiently stable for sufficiently long, you should probably call it 1.0 even if it doesn't "feel ready" for the 1.0 label.

Anyway, I think that I personally don't realistically have the bandwidth to help very much with any effort to make this happen, but I'd be in favor of it.

@CraftSpider
Copy link
Author

I already did the easy MSRV cleanups in #57, I'd be willing to take a look at some of the other things on the list when I have a moment. I'll see if I can find some Rust version usage numbers somewhere - if up till 1.30 or so is basically unused, may be worth upping to that to get #[non_exhaustive].

src/lib.rs Show resolved Hide resolved
@CraftSpider CraftSpider mentioned this pull request Feb 12, 2024
6 tasks
@CraftSpider
Copy link
Author

I added a private __non_exhaustive field to Library - the error messages will be slightly worse, but this should give us the same allowances as #[non_exhaustive] would in most cases, since outside users would now be unable to construct or exhaustively match the struct.

@CraftSpider
Copy link
Author

Any updates on this?

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.

3 participants