Skip to content
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

Add generate QR code #330

Merged
merged 4 commits into from Jul 5, 2020
Merged

Add generate QR code #330

merged 4 commits into from Jul 5, 2020

Conversation

wyhaya
Copy link
Contributor

@wyhaya wyhaya commented Jun 29, 2020

preview

#45

Copy link
Owner

@svenstaro svenstaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff! A few things:

  • I think there should be a CLI switch to disable this function (to keep the UI tidy for people who don't like the visual bloat).
  • Could you add a parametrized test for this that checks multiple values for the qrcode param and see whether you can trigger the error, too?

@svenstaro
Copy link
Owner

Also this needs a rebase.

@svenstaro
Copy link
Owner

Do you also want to tackle the task of printing a QR code in the terminal if requested via option flag? That was part of issue #45 but it should likely be done in a separate PR if you want to do it.

@wyhaya
Copy link
Contributor Author

wyhaya commented Jun 29, 2020

I think there should be a CLI switch to disable this function (to keep the UI tidy for people who don't like the visual bloat).

It may be considered to add a CLI switch, but is there too many switches at present, it is difficult for users to understand each function, maybe we can make the style smaller?

Could you add a parametrized test for this that checks multiple values for the qrcode param and see whether you can trigger the error, too?

Ok i will add as soon as possible.

Do you also want to tackle the task of printing a QR code in the terminal if requested via option flag? That was part of issue #45 but it should likely be done in a separate PR if you want to do it.

Yes, but what characters should we use? The terminal cannot be completely set to a square, because it is impossible to control the user’s line-height, it is usually difficult to scan successfully

Reference: qrcode-rust


Using " " and "██" may be closer, but it will take up more space, users need to change the size of the terminal before they can scan.

@svenstaro
Copy link
Owner

I think there should be a CLI switch to disable this function (to keep the UI tidy for people who don't like the visual bloat).

It may be considered to add a CLI switch, but is there too many switches at present, it is difficult for users to understand each function, maybe we can make the style smaller?

I definitely am worried about making the tool too complex as it touts itself as being simple. However, I think at current we can handle a --no-qr (or similar) flag just fine.

Do you also want to tackle the task of printing a QR code in the terminal if requested via option flag? That was part of issue #45 but it should likely be done in a separate PR if you want to do it.

Yes, but what characters should we use? The terminal cannot be completely set to a square, because it is impossible to control the user’s line-height, it is usually difficult to scan successfully

Reference: qrcode-rust

True, though its unicode mode works well enough for me so let's just roll with that for the time being.

@svenstaro
Copy link
Owner

FWIW the unicode string mode is gapless for me.

@wyhaya wyhaya requested a review from svenstaro June 30, 2020 03:46
@wyhaya
Copy link
Contributor Author

wyhaya commented Jun 30, 2020

I added the --qrcode option, which is false by default. Do you think it is appropriate?

Copy link
Owner

@svenstaro svenstaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking pretty good now, I like the flag.

Can you add this feature to the list of features in the README? I think it's worth mentioning. :)

Any chance you can get rid of your merge commit and make it a rebase instead? Two actual commits are fine of course but the merge commit itself is not useful.

src/renderer.rs Outdated Show resolved Hide resolved
src/renderer.rs Outdated Show resolved Hide resolved
@wyhaya wyhaya requested a review from svenstaro July 5, 2020 13:48
Copy link
Owner

@svenstaro svenstaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Great work, thanks.

@svenstaro svenstaro merged commit 205a5a4 into svenstaro:master Jul 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants