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

Fix #152: encoding name of addresses in utf-8 #166

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tomyeh
Copy link
Collaborator

@tomyeh tomyeh commented Mar 6, 2021

@close2 I fixed #152 by encoding name to utf-8+base64.

Note:

  1. It doesn't support 2.12 yet
  2. I introduced Address.sanitizedName and sanitizedAddress for application to override (which I need it).
  3. I made Address's name and mailAddress as final, so const Address is possible.
    • I know you revoked it when merging the code. But, I think the version of 2.12 is a good opportunity to drop some backward compatible code.
    • Make them final have two advantages: 1) easy to override, 2) able to use const to make the code a bit more efficient.

Item 2 and 3 are not necessary. If you don't like it you can revoke it.

var splitData = split(convert.utf8.encode(value), availableLength);
var second = false;
for (var d in split(convert.utf8.encode(value), availableLength)) {
if (second) yield _$eolSpace;
Copy link
Collaborator

Choose a reason for hiding this comment

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

was there a bug in my code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, I just refactored it a bit so I can reuse some code and constants.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't find the yield _$eolSpace in my old code.

But your refactoring looks good to me. (If it was in my code good. If not it will be added by your code)

@close2
Copy link
Collaborator

close2 commented Mar 9, 2021

LGTM

Do you want me to wait for a 2.12 version?

The null safety version would have been a perfect opportunity to change the interface.
Bad luck. I'll just bump the version to 5.0.0

@tomyeh
Copy link
Collaborator Author

tomyeh commented Mar 10, 2021

Please upgrade it to 2.12.
I don't plan to upgrade our apps to 2.12 yet.

@close2
Copy link
Collaborator

close2 commented Mar 13, 2021

Please upgrade it to 2.12.
I don't plan to upgrade our apps to 2.12 yet.

Will do.

@close2
Copy link
Collaborator

close2 commented Mar 28, 2021

Hi @tomyeh !

I've created a PR where your changes are merged with the current 4.0.0 version:
#170

This PR is currently more or less untested.

Your feedback is welcome.

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.

encoding name in Address
2 participants