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

Getting Multibase ready for IETF review (align with IANA terminology and registry governance style) #109

Conversation

bumblefudge
Copy link
Contributor

@bumblefudge bumblefudge commented Jul 4, 2023

For pre-PR context, see original issue on W3C-CCG discussion repo here and multiformats#65

@bumblefudge bumblefudge closed this Jul 4, 2023
@bumblefudge bumblefudge reopened this Jul 4, 2023
@bumblefudge bumblefudge changed the title first-stab-at-closing-upsteam-issue-4 First stab at address jyasskin suggestions on w3c-ccg/multibase#4 Jul 4, 2023
multibase.csv Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
multibase.csv Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Jul 5, 2023

Could you try and column-align both forms of the table please? Removing the word "Unicode" in the second column would help compact it a bit if that's not unreasonable to remove it.

You're also welcome to note in there that the practical bases only have a single-byte prefix indicator, and that implementations may choose to only concern themselves with the single-byte case. The emoji one just pushes the boundaries a bit but it's not expected that it gets much practical use and most people wouldn't be upset if an implementation didn't use it.

@bumblefudge bumblefudge changed the title First stab at address jyasskin suggestions on w3c-ccg/multibase#4 Second stab at address jyasskin suggestions on w3c-ccg/multibase#4 Jul 18, 2023
@bumblefudge bumblefudge changed the title Second stab at address jyasskin suggestions on w3c-ccg/multibase#4 Getting Multibase ready for IETF review Jul 18, 2023
@bumblefudge
Copy link
Contributor Author

(Dear merger - please squash these before merging, lots of little commits)

@bumblefudge bumblefudge changed the title Getting Multibase ready for IETF review Getting Multibase ready for IETF review (align with IANA terminology and registry governance style) Jul 18, 2023
README.md Outdated
base64urlpad, U, rfc4648 with padding, default
proquint, p, PRO-QUINT https://arxiv.org/html/0901.4016, draft
base256emoji, 🚀, base256 with custom alphabet using variable-sized-codepoints, draft
code, Unicode, encoding, description, status
Copy link
Member

Choose a reason for hiding this comment

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

I'm unsure if "code" is the right word here. It's more like the "ASCII representation", so perhaps just "ASCII"? I would then move it into the second (column) as the Unicode is really the canonical thing.

Choose a reason for hiding this comment

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

The goal is to communicate that the first column is the display name for the encoding and the second column is the machine readable prefix.

code => display name for humans
Unicode => machine readable prefix (runes)
utf-8 => machine readable prefix (bytes)

I don't know if these name communicate the conesepts well or not.

Copy link
Member

Choose a reason for hiding this comment

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

In the newest version the columns are called:

Unicode => The thing that implementations should care about
Character => Display name what users see

Is that better?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
Comment on lines 30 to 40
- [multibase](#multibase)
- [Format](#format)
- [Multibase Table](#multibase-table)
- [Reserved](#reserved)
- [Status](#status)
- [Multibase By Example](#multibase-by-example)
- [FAQ](#faq)
- [Implementations:](#implementations)
- [Disclaimers](#disclaimers)
- [Contribute](#contribute)
- [License](#license)
Copy link
Member

Choose a reason for hiding this comment

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

I don't get the ToC reformatting, but it's not a blocker for me.

multibase.csv Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

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

There is one little comment left from me, but I don't consider it a blocker.

Before this is merged, I'd like to see an approval from @rvagg and @Stebalien as well (or at least a "it's OK to be merged").

@vmx
Copy link
Member

vmx commented Aug 16, 2023

Just for everyone's information: one big change here is the 0x00 is no longer considered a Multibase. Though we keep it as "reserved" for now (perhaps we should remove it completely). This aligns with the comments at #85 (comment) and also what we talked about the IPLD community/sync call.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Cursory LGTM (I didn't read it in-depth, just looked at the high-level changes).

I was initially a bit concerned about specifying this in terms of unicode code-points, but that shouldn't tie us to any specific encodings, it just adds some clarity. In theory, any encoding can (and likely is) mapped to unicode code-points.

Deprecating NUL is definitely the right thing to do.

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.

Minor change on README and major concerns with the issue templates; can you tell I'm not a fan of issue templates? If we must have issue templates, can we make them less prickly for the user and more actionable for maintainers? In general I'm happy to answer people's free-form questions in issues and would prefer people self-serve when it comes to submitting stuff, that's what PRs are for.

.github/ISSUE_TEMPLATE/BUG-REPORT.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/BUG-REPORT.yml Outdated Show resolved Hide resolved
@@ -0,0 +1,55 @@
name: "Interest in Implementing - New Multibase Library or System"
description: Express interest in possible new multibase library or system
Copy link
Member

Choose a reason for hiding this comment

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

From the README:

If you'd like to switch a project over to multibase, whether by creating a new multibase implementation or building on one of those listed above, please file an issue in this repository using the "Interested in implementing" issue template.

This isn't really actionable and I'm unsure what we're supposed to do with these issues. Just a nod and and ignore? I'd like these to be actionable and not leading to cruft in the issue tracker since we already have a tendency toward cruft-buildup.

I think the purpose here might be to get a new implementation/library/system onto a list somewhere, can we just shunt people to open a pull request to add their thing? The "express interest" only really tells me that the issue opener is going to open a new issue telling us what they have in their head, which may be awesome, or may be terrible, but it's not really actionable without a thing or an explicit question--which is fine, but this isn't a template for a question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this be better handled by a GH "Discussion"? TBH I wanted an "expressed interest" tracker somewhere for IETF purposes, because the perennial question of standardization is "for whom is this actually useful outside of the PLN/IPFS community that spawned it?"...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the interest of not holding up this PR, I have just cut that template for now and will open a new PR adding it as a Discussion template.

I only realized just now that discussion templates are also a thing! Separate PR incoming.

@@ -0,0 +1,69 @@
name: "New Registration"
description: Express interest in registering a new encoding
Copy link
Member

Choose a reason for hiding this comment

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

If would also like to reserve a prefix for compatibility, please file a separate issue in this repository using the "New Registration" issue template.

This one's a little better, but I'd still prefer this in PR form rather than this asking us to do the PR for them; I'm not really a user of github issue templates (nor particularly a fan! I did my time in JIRA hell already so I can do without the extreme structure), do we have the tools to shunt people toward a PR with all of these things filled out there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will rework the semantics so that it's more of a placeholder. Maybe something like "open this issue so we can give you timely feedback before you start, but please open a draft PR soon and work there until you can close the issue you just opened" would work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the insertion of this checkbox make it explicit enough?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I guess -- what I'd like to avoid here is a collection of poorly maintained opened issues with no clear path to getting them closed. If we permission people opening unactionable issues, then we can't take action and have no clear path to closing them.

I think it's fine to have issues opened flagging intent to register, but we have to acknowledge that there's a limit to how long those will be held.

README.md Outdated Show resolved Hide resolved
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.

I guess we've tinkered enough, thanks for your patience and persistence @bumblefudge
anything else @vmx?

@vmx
Copy link
Member

vmx commented Aug 23, 2023

I think it's good to be merged. Thanks @bumblefudge for the hard work and @rvagg for being so thorough about the issue templates, I totally agree that it's good to reduce non-actionable issues.

@rvagg
Copy link
Member

rvagg commented Aug 24, 2023

I hope a squash merge is OK @bumblefudge, lots of info in these commits but there's too many. If you'd prefer to keep more context when we can force push it out and put some new chain in, but 33 commits is a bit too many!

@rvagg rvagg merged commit 3a13be7 into multiformats:master Aug 24, 2023
@ben221199
Copy link
Contributor

I see a extra column with codepoint information. That is nice. The status reserved is exactly what I had in mind, but maybe the column name could be state instead of status. Also I don't know if having none as encoding name is the right way to do. I think I prefer it being empty. Also I think that a relevant description is more useful than (No base encoding).

@ben221199
Copy link
Contributor

Also, identity is removed for 0x00. Is something planned to add it back with another codepoint?

@rvagg
Copy link
Member

rvagg commented Aug 24, 2023

See discussion above re NUL, we've all agreed it was a historical accident and the only place it's really used is in dag-cbor where it's recorded in the spec as something of a historical accident. It's still there if people really want to use it, but there's not really a good justification for it.

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

6 participants