-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
base: main
Are you sure you want to change the base?
Conversation
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 Is there an estimate of when it is going to be merged? |
@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. |
@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 |
@NickCraver Thanks for updating 👍 |
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.