-
Notifications
You must be signed in to change notification settings - Fork 1
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
FS implementation #11
Conversation
808ad1e
to
ec227fe
Compare
I'm going to say this is pretty much done unless we make a major change.
The current implementation takes a middle-of-the-road approach. ¯\(ツ)/¯ |
It turns out that some features like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review, because I've started looking, but probably won't finish thinking it through before bed:
- This looks awesome overall!
- Thank you for the excellent comments in the PR about considerations! That's really great to have recorded and shared.
- Thank you for the massive amounts of tests!
- I'm of the same thinking of wobbling about whether to put the FS-emulation in a separate package. Mostly I'm looking at the godoc as a decision guide.
- Pro of having a separate package: less likely that a user will get confused by seeing
File
,Hunk
, andDocument
all in the godoc of a package. (Also, the type namedFile
wouldn't end up next to all the functions named*File*
, which are doing something quite different.) - Con of having a separate package: having a second package is arguably overkill.
- Pro of having a separate package: less likely that a user will get confused by seeing
- Oh dear. I just remembered how new the
io/fs
package is.- ... well, meh. It's been out several releases by now. I think I was explicitly resisting using it when I first wrote this library, but now it doesn't seem unreasonable to ask for anymore.
- Multidoc is... spicy. I had to look twice at it before I grokked what's going on there. I might wanna look at it a third time after sleeping on it.
- This seems like a thing that may need some recommendations around usage, if we introduce it. It's not immediately clear to me what it's for (or honestly if it's a good idea at all... because it seems like a powerful way to break the "narrative" UX, which doesn't necessarily feel like a win).
- In code organization... it's nothing to do with the stuff about fitting the
io/fs
interfaces, yes? so should probably be in a different file - ... if not separate PR
fs.go
Outdated
// Opening an empty path will return the root directory for the document. | ||
// This is different than the fs.ValidPath special case of using "." as the root path. | ||
// The testmark document treats "." and ".." the same as any other character. | ||
func (doc *Document) Open(name string) (fs.File, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer to flip this around into a func OpenAsFile(doc *Document) (fs.File, error)
. Slightly less coupled; I don't like object-oriented flavor in general. Is also what would happen if we pulled the FS bits out into a subpackage.
(The function name not being important, just the first thing that came to mind. Something shorter would probably be better, if you can think something that fits.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this pattern is part of the fs.FS
interface. Moving it all to its own package has some advantages here.
fs.go
Outdated
// size is generally the number of directory entires or the length of the file in bytes. | ||
// We have to choose one or the other because files and directories can overlap | ||
// In my opinion it's best to go with file length, so directories will always have a size of zero. | ||
size: size, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe worth a quick else
clause up around where the size for files is being figured?
The general awkwardness wherein hunks and "dirs" can colide doesn't go away, ofc, but less zeros seems better, and it's easy enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now the zero is the easiest way to differentiate between HunkExists and HunkDoesNotExist and I think there's some value there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I... don't think that's obvious enough to be valuable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
¯\(ツ)/¯. Alternatively you round trip back to DirEnt
and poke the Hunk
value directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add a flag directly to the File type specifically for that but it still requires a minimum of one type coercion.
A quick test in play shows that |
Yeah. I think some mild documentation warnings are fine and also sufficient. The contract is a fairly handwavey "approximately filesystem-like semantics are offered opportunistically for data that roughly matches appropriate conventions", more or less... so warning people against naming a hunk "." seems pretty mild. |
22925f3
to
9c9bb52
Compare
(The multi-doc part is no longer here, so reviewing what's left is a lot easier.) FS stuff is in a subpackage now. Also good. Derisking by reducing visible change in the most core API zone, and compartmentalizing. 👍 It also marks itself as experimental, which seems wise. (It might not change much, but maybe little edge cases will get polished after seeing interactions in the wild, so it's good to document that possibility.) LGTM. (I'm gonna use the squash merge on this one because it was a bit of a journey :)) |
This is a working branch for implementing the fs.FS interface.