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

Options: support changing lib-ver #2547

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

NickCraver
Copy link
Collaborator

@NickCraver NickCraver commented Sep 10, 2023

This adds similar support from LibraryName to LibraryVersion so that consumers can override both if they so choose - this works in all the same places, DefaultOptionsProvider, etc.

Resolves #2533.

This adds similar support from LibraryName to LibraryVersion so that consumers can override both if they so choose - this works in all the same places, DefaultOptionsProvider, etc.
Note: this cleans up Add: vs Adds: too since we had it differing even in the upcoming release.
We want these fast, but 50ms allowance under load on test runners is too strict - eat some time for better stability.
@NickCraver NickCraver marked this pull request as ready for review September 10, 2023 13:11
@shacharPash
Copy link
Contributor

@NickCraver Is there an estimate of when it is going to be merged?

@mgravell
Copy link
Collaborator

mgravell commented Sep 27, 2023

@shacharPash I believe the snag on this one is the static -> virtual, which is a big change to make in a minor; in voice call, we discussed options here, including breaking the change into two parts, so that there's no single hard break - for example, in version X we could do:

- protected static string LibraryVersion => Utils.GetLibVersion();
+ [Obsolete("words, basically: stop using this")]
+ protected static string LibraryVersion => DefaultLibraryVersion;
+ protected static string DefaultLibraryVersion => Utils.GetLibVersion();

then in change Y

- [Obsolete("words, basically: stop using this")]
- protected static string LibraryVersion => DefaultLibraryVersion; 
+ public virtual string LibraryVersion => DefaultLibraryVersion;

If we do go that route, it'll take at least 2 deploys to get it released. However, I'm open to other options. Just; the hard break here is not ideal, even if it is a very niche API that most folks won't be using.

The other option, of course, is to just use a different name, i.e.

  protected static string LibraryVersion => Utils.GetLibVersion();
+ public virtual string EffectiveLibraryVersion => LibraryVersion;

Which is a little ugly, but much quicker to ship.

@NickCraver
Copy link
Collaborator Author

@shacharPash After talking about this and looking through it - I believe it will be a 3.x addition due to the need for a break here. We have many calls to Utils directly to get this (that can't necessarily use options), so having an intermediate non-breaking solution which we'd still not want to break for 3.x either is pretty messy.

@NickCraver NickCraver marked this pull request as draft October 4, 2023 12:52
@shacharPash
Copy link
Contributor

@NickCraver Thanks for updating 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set LibraryVersion in ConfigurationOptions
3 participants