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: add new emoji codepoint for Base256Emoji 🐉 #98

Merged
merged 2 commits into from
Jun 16, 2022

Conversation

Jorropo
Copy link
Contributor

@Jorropo Jorropo commented Jun 13, 2022

One of the values of base256emoji was not actually an emoji.

Here there be dragons instead !

See multiformats/go-multibase@df5b7bc#r75683795 for context.

Pls do not rebase (merge instead), I'm lazy and don't want to have to update the submodule once again.

@Jorropo
Copy link
Contributor Author

Jorropo commented Jun 13, 2022

Questions, none of the tests contain ☉ or 🌠, should I make a new one or can I rely on implementations getting this right ?

rfcs/Base256Emoji.md Outdated Show resolved Hide resolved
@Jorropo Jorropo force-pushed the emoji-emoji-base branch 7 times, most recently from 1c4cb24 to ae04ea4 Compare June 13, 2022 20:48
@Jorropo
Copy link
Contributor Author

Jorropo commented Jun 13, 2022

/cc @vmx (you commented on the commit's thread)

Jorropo referenced this pull request in multiformats/go-multibase Jun 13, 2022
This include fixes for UTF-8 as well as base256emoji encoding (an encoding which actually use UTF-8).
@Jorropo Jorropo changed the title fix: add new emoji codepoint for Base256Emoji fix: add new emoji codepoint for Base256Emoji 🐉 Jun 13, 2022
@rvagg
Copy link
Member

rvagg commented Jun 14, 2022

Not a fan of the "backward compatibility" thing tbh, I think I'd prefer we just rip it out and get a fix into go-multibase as if it never existed the old way; we surely have permission to be a bit lose with a troll multibase. The JS impl hasn't been released and I'd lean to not even adding it there either way.

@Jorropo Jorropo force-pushed the emoji-emoji-base branch from ae04ea4 to e75cd09 Compare June 14, 2022 16:43
Jorropo added 2 commits June 14, 2022 18:48
One of the values of base256emoji was not actually an emoji.

Here there be dragons instead !

See multiformats/go-multibase@df5b7bc#r75683795 for context.
@Jorropo Jorropo force-pushed the emoji-emoji-base branch from e75cd09 to 4c8344e Compare June 14, 2022 16:48
@Jorropo
Copy link
Contributor Author

Jorropo commented Jun 14, 2022

@rvagg I have removed it. It's now just breaking to 🐉 !

If anyone wants to do changes to the set (like propose your favorite emojies), pls do it now.

I see that now that it's merged people are asking taking a look at it. Well we have to break it anyway, might as well bundle everything now.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

🚢 good to go, anyone else?

@rvagg
Copy link
Member

rvagg commented Jun 16, 2022

fine, let's do this (again)

@rvagg rvagg merged commit 506c6df into multiformats:master Jun 16, 2022
@Jorropo Jorropo deleted the emoji-emoji-base branch June 16, 2022 11:15
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.

4 participants