Path clarifications #47
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Primarily, being commital and direct about how special values, erm... *aren't* -- namely, "/" is a valid segment, and so is empty string -- and confessing how much tricky work that makes. Several methods are now more upfront about how much they *do not do good things* on such values. Added some new constructors for Path which *do* work for all possible values. (You could've handled such tricky values with a series of Join calls, previously, but that would be ergonomically grueling.) These facts are all defacto 'true' to the best of my knowledge now. However, the IPLD specs repo is a bit on the quiet side about them at present (precisely because it's such an irritating little nest of edge cases and fun stuff)... and so the comments are also littered with "this may change" warnings. Still, it's better to be accurate about what the code does and does not do in its current state. I'd *like* to formally specify an escaping system and canonical string encoding for paths. That should start in the IPLD Specs repo, though, and involve fixtures, so I won't start it here now. Thanks to @ribasushi for the kick in the shins that these docs needed work and clarifications.
It's not very often we care about these things in Go, and when we do, this stance of "string is really just bytes; guard it yourself" is the community norm... so, these docs should *almost* go without saying. However, since IPLD is all about cross-language consistency, and we may have people from other programming language ecosystems reading our docs, it seems good to be as clear and explicit as possible.
I... honestly hadn't even realized this before; I just noticed it when making a refactor that would produce this end state, and thought "oh, that's silly". Evidently... it's fine, since this has already been the case, and not been problematic! Alright then. Apparently it's not too silly after all.
warpfork
force-pushed
the
path-clarifications
branch
from
c7b11de
to
a48c9db
Compare
Yep, it makes a lot of creation by struct uglier, as you can see in the diff to the test file. Fortunately? Nobody outside the package is allowed to do that anyway, so, we can pretty much ignore the ergonomic. (It'll still look uglier if someone's looking at stuff with spew or go-cmp or some other library like that which peeks naughtily into unexported fields, but that's hard to do much about.) We could also address this by adding a discriminant field to the PathSegment struct. (If we wanted to use uint internally, we'd probably have to do that, in fact. Either that or start using some other alarmingly magical number like UINT_MAX. Ygh.) I didn't. I don't think we handle lists larger than 2 billion elements particularly well anyway... so, adding more words of memory to PathSegment to support that case seems like an entirely losing trade.
Codecov Report
@@ Coverage Diff @@
## master #47 +/- ##
==========================================
+ Coverage 72.21% 72.61% +0.41%
==========================================
Files 50 50
Lines 3162 3209 +47
==========================================
+ Hits 2283 2330 +47
+ Misses 789 788 -1
- Partials 90 91 +1
Continue to review full report at Codecov.
|
ribasushi
reviewed
Feb 25, 2020
There's already explicit sections for empty string and slash cases, on the basis of how likely those are to provoke questions (even though the content is essentially to highlight how *not* exceptional they are); it makes just as much sense to do the same call out for null bytes. Original text started in https://github.com/ipld/go-ipld-prime/pull/47/files#r384127483 . Co-Authored-By: Peter Rabbitson <ribasushi@leporine.io>
Previous commit with suggestions from reviewer made me look at this line again and realize it was *too* specific.
warpfork
force-pushed
the
path-clarifications
branch
from
ae08d2d
to
8093be7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Several significant clarifications to Path docs.
Primarily, being committal and direct about how special values, erm... aren't -- namely, "/" is a valid segment, and so is empty string -- and confessing how much tricky work that makes.
Several methods are now more upfront about how much they do not do good things on such values. I'd like to fix these methods, but to do that, we should formally specify an escaping system and canonical string encoding for paths. That work should start in the IPLD Specs repo, and involve fixtures. Once that is tackled, I will (very!) gladly start fixing these methods to operate accordingly, and get rid of all the sharp-edges warnings.
Several methods also now include comments about whether they will enforce string character encoding or normalization on their function boundaries. They won't. (This is certainly the norm in Go -- the stance of "string is really just bytes" is ubiquitous in the gopher community -- so it may seem not worthy of wasting docs on to other gopher readers. However, since IPLD is all about cross-language consistency, and we may have people from other programming language ecosystems reading our docs, it seems good to be as clear and explicit as possible.) I suspect this will remain the case (it's turned out tricky and work-makey to try to specify anything else; subtracting the countably-infinite domain of non-normalized strings from the countably-infinite domain of all possible byte sequences makes for some very nasty and pernicious expressibility and round-tripping issues, long story short).
Thanks to @ribasushi for the kick in the shins that these docs needed work and clarifications. The previous docs were written a fair while ago when we knew less things less confidently about how the system and specs would evolve, so these clarifications were badly needed.