-
Notifications
You must be signed in to change notification settings - Fork 191
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
renaming rfc4122 functions #754
Conversation
as per Also suggesting checking on my other PR #753 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @Mikopet. I can see the motivation for doing this, especially since the old RFC is now obsolete in favour of 9562. I think the name rfc
on its own isn't specific enough to tell what it should be so we could make it rfc9562
instead. The old RFC got almost 20 years of mileage out of it so I don't think we're at a big risk of needing to do this a second time anytime soon.
Having said that, I'm not fully convinced we should do this. I personally find these kinds of pure-renaming refactorings mostly just a nuisance as a user. I think it's something to keep up our sleeves though if we find a better API in the future to push users to.
I agree, sort of. name
No disagree here, this PR indeed is some bikeshedding. More like an attempt to push the crate in the direction for some cleanup efforts.. as it is a little bit lorn... yet, it is very much on the important side of all crates. (top 100) I do have some new ideas for a new API, would mean a major release. I will share it on the relevant PR soon (when the PoC is working as expected) so, back to this PR. What do you think about a rename to |
I think we can revisit this after I finish #755, it ends up needing to add to the timestamp API to support the more sophisticated counter requirements in v7 UUIDs. |
#755 is merged now. I think if we rename these, we could possibly use the name I don't think this needs to block our next release, because it doesn't add any new methods we'd want to deprecate. |
6f6cafb
to
123daaa
Compare
Renamed the Also I attempted to rename the Variant field, with backward compatibility. It adds a slight plus complexity, but will disappear when we delete deprecated items. It may not be fully super BC... but I guess for that User has to do some very niche edge case what they probably should not do any ways. Anyways, feel free to cherry pick... or just close if it's not needed. (deprecation warnings points to future version 1.9.1) |
Thanks @Mikopet. I think the name |
Indeed, but not that much. My reasoning is: while the RFC mentions "Gregorian" many times (so they call it by name), the variant field just says:
I agree on that, it is indeed very niche. This is more likely a thought experiment and an attempt to put this things in order in a compatible way. To be honest, I don't really like the current Variant/Version enums. They are not representating the things written in the RFC very well. I would suggest a more nearby approach to it. I don't have ready implementation for it, but some idea I do have. I'm happy to jump on it, if you think it could be a good direction:
Anyhow, I updated the commits! |
Hey!
My second contribution is a small rename of certain obsolete function names. (namely, contains rfc4122)
The changes are cosmetic, does not carry real value...
... but it has some documental changes and additions regarding deprecation messages.
Majority of the functions were private, but there was also a few public ones. This latter ones got proxy functions in place to not break BC, with the proper deprecation warning.
During my testing it works as expected... also, I took the liberty and put the next version number into the deprecation messages, indicating there will be a patch version published.
When we will have version 2.0, we could clean up deprecated features.
Preferably should be merged after #753