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

proposal: x/text: expose MIBenum via identifier package #47577

Closed
rykov opened this issue Aug 6, 2021 · 17 comments
Closed

proposal: x/text: expose MIBenum via identifier package #47577

rykov opened this issue Aug 6, 2021 · 17 comments

Comments

@rykov
Copy link

rykov commented Aug 6, 2021

I am having trouble finding a way to store and look-up a specific encoding by MIBenum. It would be much more convenient and compact to have access to the MIBenum, rather than using a string name. I would like to start a discussion about exposing MIBenum via the ID() method for Encoding by moving identifier.Interface, identifier.MIB, out of an internal package, as well as exposing FindMIB for ianaindex.Index.

MIB enum is part of the IANA standard as described here:
https://www.iana.org/assignments/character-sets/character-sets.xhtml

@gopherbot gopherbot added this to the Proposal milestone Aug 6, 2021
@rykov rykov changed the title proposal: x/text: explose MIBenum via identifier package proposal: x/text: expose MIBenum via identifier package Aug 6, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Aug 6, 2021
@ianlancetaylor
Copy link
Contributor

CC @mpvl

@rsc
Copy link
Contributor

rsc commented Oct 13, 2021

In what context do you need to look up an encoding by MIBenum?

@rsc
Copy link
Contributor

rsc commented Oct 13, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) Oct 13, 2021
@rykov
Copy link
Author

rykov commented Oct 20, 2021

In what context do you need to look up an encoding by MIBenum?

@rsc Not so much for look-up, but rather for storage - storing a number is much more compact.

@rsc
Copy link
Contributor

rsc commented Oct 27, 2021

What is the exact API you are proposing? What packages would change and how?

@rsc
Copy link
Contributor

rsc commented Dec 1, 2021

ping @rykov; what API are you suggesting?

@rykov
Copy link
Author

rykov commented Dec 2, 2021

@rsc Sorry for the delay. I've prototyped the proposal in golang/text#30

@rsc
Copy link
Contributor

rsc commented Dec 8, 2021

The new API is

func (x *Index) FindMIB(mib identifier.MIB) (encoding.Encoding, error)

But identifier is an internal package. We would need to export MIB from some package (probably not all of identifier).

@rykov
Copy link
Author

rykov commented Jan 2, 2022

@rsc Right. I'm only exporting identifier.MIB and identifier.Interface as aliases, the rest stays internal.

@rsc
Copy link
Contributor

rsc commented Jan 12, 2022

It's very difficult to document those as aliases. We'd need to find some conversion back and forth to hide the existence of the internal package. Any ideas?

@rykov
Copy link
Author

rykov commented Jan 19, 2022

@rsc I don't fully understand: are you suggesting creating two separate types to wrap the internal ones, or do you think that moving the two types out of the internal package into the public package is the better way?

@rsc
Copy link
Contributor

rsc commented Jan 19, 2022

I looked some more at these packages. I think if we were going to add this API we would want to move x/text/encoding/internal/identifier to x/text/encoding/identifier first. I am not sure whether we should do that though.

@rykov
Copy link
Author

rykov commented Jan 21, 2022

I can try exposing a minimal API via x/text/encoding/identifier, and keep the rest hidden in x/text/encoding/internal/identifier or x/text/encoding/identifier/internal.

@rsc
Copy link
Contributor

rsc commented Sep 21, 2022

@mpvl and I talked about this. We spent a while trying to figure out where to define the MIB type, and we ended up back at "why is this necessary?" When I asked earlier the answer was "Not so much for look-up, but rather for storage - storing a number is much more compact." It seems like if you just need a mapping from name to compact form for storage, it would be easy to define a local one instead of reusing the MIB numbers, which suggests we shouldn't make any API changes here - the justification is not strong enough.

@rsc
Copy link
Contributor

rsc commented Sep 28, 2022

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rykov
Copy link
Author

rykov commented Sep 28, 2022

@rsc We've taken a different direction for text encoding in our project, so we no longer needs this feature. However, my 2¢: given that MIBenum is part of the standard, I disagree that the solution is to keep the mapping internal, and have a consumer of this library reimplement their own equivalent local mapping.

@rsc
Copy link
Contributor

rsc commented Oct 6, 2022

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed Oct 6, 2022
@golang golang locked and limited conversation to collaborators Oct 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

4 participants