-
-
Notifications
You must be signed in to change notification settings - Fork 512
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
refactor(biome_deserialize): use enumflags2
#3529
Conversation
CodSpeed Performance ReportMerging #3529 will improve performances by 6.45%Comparing Summary
Benchmarks breakdown
|
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 your contribution!
It looks good to me. I suggested some change to improve the code.
I noticed the care of reducing breaking change as much as possible.
I wonder if we could use more consistent naming: DeserializableType
instead of Types
and DeserializableTypes
instead of VisitableType
.
We could still avoid most breaking changes by using a type alias:
#[deprecated(since="0.7.0", note="please use `DeserializableType` instead")]
pub type VisitableType = DeserializableTypes;
We should also update bione_deserialize_macros
if we change the type name.
Also, let us know that you are working on #3157, I was going to assign it to me ^^ |
Thank you for your guidance! |
Hey @RiESAEX, thank you for your contribution! I think what @Conaclos meant was to write a comment on the issue to avoid duplicate work! You're free to continue the work if you want, you should just leave a comment saying where. If you wish to play more, the |
Once everything is green on the CI, we will merge the PR :) |
Summary
Part of #3157
Test Plan
The current CI should pass