-
-
Notifications
You must be signed in to change notification settings - Fork 16
docs(stackable-versioned): Add section about hint(...) #1128
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
Open
Techassi
wants to merge
1
commit into
main
Choose a base branch
from
docs/stackable-versioned-hints
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
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.
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.
My understanding is that this is done by using whatever is defined for
upgrade_withanddowngrade_withon the field itself. Is it worth mentioning that here for clarity?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.
Both
upgrade_withanddowngrade_withare meant to give the user complete control of the conversion. There are however cases, where the wrapped type already implements conversions (automatically generated or manually implemented), but the macro cannot reason about the wrapped types. An example:hint(vec): Generated code will try to convert aVec<T>by callingold_field.into(). This will fail, because there is no appropriateFromimplementation forVec<T>, but only forT.hint(vec): The generated code now maps each value of theVec<T>, which will result in the correctVec<U>afterwards.Currently, both
upgrade_with/downgrade_withandhintkind of "conflict" with each other/produce unexpected code. I've already raised an improvement for this. I think the expected behaviour should be like this:hint, onlyupgrade_with/downgrade_with: The conversion function(s) need to deal with the complete type.upgrade_with/downgrade_with: The inner values are mapped using the defined conversion functions instead of the defaultInto::into.Do you want any of this mentioned in the docs?
Uh oh!
There was an error while loading. Please reload this page.
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.
What confused me was having nested versioning, whereby I have
upgrade_with/downgrade_withon the field itself, and a second conversion (instead of ahint) that basically called those functions in a manual mapping step in the "parent" struct. Soit would be a case ofI'm now usinghint(on the parent), implicitly callingupgrade_with/downgrade_withon the child. At least, I think that is what is happening.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.
(Specifically,
upgrade_with/downgrade_withon a field in the gitsync struct, and ahinton the the parent struct that includes the gitsync struct as a field).Uh oh!
There was an error while loading. Please reload this page.
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.
That's what I basically described above, because the macro cannot reason about wrapped types. Therefor it would complain about missing
Fromimpls, which then led you to implement manual conversions, even though they weren't needed (with the usage ofhint).Both these functions are not needed/currently used by
hint. Here is an example of the generated code:You can expand this code yourself by either using
cargo-expandor via rust-analyzer by placing your caret on the top-level#[versioned]macro, opening the command prompt (usuallyCtrl+Shift+Pin VS Code), and then selectingrust-analyzer: Expand macro recursively at caret.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.
After we discussed this yesterday, I would maybe go with something like this: