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
Address to optimized QR string #581
Merged
Merged
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
b9d5200
Access Display and Formatter with fmt:: like in other places
RCasatta d18554e
Address to string conversion optimized for qr codes
RCasatta cac3f46
improve to_qr_string doc
RCasatta 104836a
implements alternate formatting for address
RCasatta 85ae82f
use the char trick to avoid allocation
RCasatta bc406bf
Use &mut dyn fmt::Write instead of bool
Kixunil 3158ced
document alternate formatting
RCasatta 0a91496
rename to_qr_string into to_qr_uri returning also the schema
RCasatta File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
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
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.
I believe dispatching here instead of using
bool
is a bit faster, but probably not something to worry about.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.
Idk if that's what you mean (I think you mean putting the
if
here), but also why supply a boolean derived fromfmt
when you supplyfmt
?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.
the naive reason is that fmt is taken as something that impl fmt::Write, not as Formatter, so it's lacking
alternate()
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.
@sgeisler yes, I meant putting the
if
there as was written before.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.
not sure how to put the
if
as was before since something like this won't workand other solutions I can think about are too verbose
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.
Wait, wasn't your initial concern essentially avoiding a single function call in some cases? Always using dynamic dispatch sounds even worse (but maybe the compiler will figure it out? but the same cou). I still see that this is more elegant than having a boolen argument.
Also: we are horribly over-engineering this PR 😆
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.
I only pointed out that evaluating
if
multiple times is not great without realizing it can't be fixed without adding bloat. Thus dynamic dispatch is probably preferable. Manually-implemented dynamic dispatch has no benefit over compiler-implemented dynamic dispatch and the latter has less boilerplate, thus should be preferred.Yeah, we're overengineering, I personally enjoy it from time to time. 😄
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.
In this example #581 (comment) there is also a lifetime issue not only a dispatching issue...
I am not sure how to go ahead:
OptionallyUpperWriter
with thebool
UpperWriter
andif fmt.is_alternate()
with the last two lines present in both of the if branchUpperWriter
andOwnedWriter
I personally still prefer 1)
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.
Ah, didn't realize the lifetime issue before. There's still a useful Rust trick to fix it, made a PR against your branch. :)
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.
very nice! merged