Support for custom default fuction with owned pattern #315
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There are still a lot of tests failing, but my usecase works. Now I just have to fix all the corner cases. Also tests for the new feature are still missing.
this still leaves tests in all other packages including tests for internals in derive_builder_core
turns out having a todo in the test data generation is not good if you want the tests to pass :)
conversion is only used to disable the default value if the enum is not OptionOrDefault. It's cleaner to just use an enabled flag for that.
|
Woops. Turns out my simplified testcase isn't actually good enough, so I will have to do some more work on this. I'll leave this as a draft PR for now. |
|
Ok, I spent the last month making sure that I did not miss anything and properly testing, this. So it should be good to merge. |
|
I'm not sure when I'll have time to review this, but I have it on my to-do list. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a fix for #298
I finally got back to my hobby project where I need this so I got around to implement this feature/bug fix.
I've implemented the solution I described in that issue so I won't repeat it here again. If there are any open questions I'm happy to answer them or adjust this PR.
No unwrap_or does not work because it expects a
Tand the default we have is an Option. Either we use or and than unwrap or we can useunwrap_or(default.unwrap()). I personally think thator.unwrapis a bit nicer.PS: I know that the rust 1.59 tests are failing right now. "Syn" just published a new version "2.0.55" that requires rust 1.60.
However on my fork the 1.59 tests succeeded with the old syn version.
PPS: I know that are a lot of commits. The best solution is to just squash them I think but I am open to rebasing them into a few usefull commits if you prefer that. This is a rather large change after all.