Export cephfs List and Names functions #326
Conversation
The current implementations for cephfs's list and names functions are non-exported functions although they do not modify any private fields. The functions would be of much more use to the community as exported functions. Signed-off-by: Lincoln Thurlow <[email protected]>
Signed-off-by: Lincoln Thurlow <[email protected]>
dirEntries need to be a type. It makes it easy to call Names(), but this was only used in testing. As a helper function, it would be better to export the already exportable DirEntry as a list. Signed-off-by: Lincoln Thurlow <[email protected]>
|
@isi-lincoln I remember having a conversation about exporting the list function so I went through my mails and found this. |
|
I had originally planned on having these be exported functions but I was talked out of it (as least for the short term). So, I am certainly OK with exporting them in a general sense. Having someone who isn't me want to use them demonstrates there's some demand for it. Issue #239 is certainly valid, but it's a lot more work. It would involve wrapping the native types in a new go-compatible interface. I guess what the (meta-) question comes down to, is are these simple direct calls useful enough on their own, or should we require all convenience functions to match those in the go os package. Because I already once thought to export them I'm OK with taking this simple approach first (with feedback), but I'll admit that I am not 100% objective in this case. :-) I'd like to hear what others think too. In the meantime I'll offer some concrete feedback on the PR as-is. |
| @@ -222,7 +217,7 @@ func (dir *Directory) List() (dirEntries, error) { | |||
| } | |||
|
|
|||
| // Names returns a slice of only the name fields from dir entries. | |||
| func (entries dirEntries) Names() []string { | |||
| func Names(entries []*DirEntry) []string { | |||
There was a problem hiding this comment.
If we're going to generalize the function we should probably make it generalize for both the readdir (DirEntry) and readdir_plus (DirEntryPlus) cases. We can probably do something like:
type NamedEntry interface {
Name() string
}
func Names(entries []NamedEntry) []string {
// ... impl ...
}
|
Sorry for the very late response, I've found that these emails were getting sent to spam. So I'll defer to the either of your opinions. I understand the good and bad of doing this, but as the functionality was there, and it was what I was looking for, rather than re-implement it myself, I thought it would be best to export them. Per the discussion on #234, I can understand the use case for that as well. For myself, I am using Let me know and I can move the PR towards that goal, or Close. Thanks for the feedback apologies again for neglecting this. |
|
No worries, and that's not a particularly late response as far as I'm concerned. @nixpanic I am of the opinion to go forward with this PR (plus some cleanups I previously suggested) but would like to have your opinion on it too, as you seem more inclined to the os-module style approach last we spoke. |
|
Ok, if others really like to have this, I won't object to it. Just make sure to implement a (non-)interface similar to what Golang offers in the os package. Or do not use the same names for functions/types so that it is clear this is a different API. We also might want to implement a Golang matching API in the future, so there should be no conflicts. |
|
Acknowledged. I'll ping again when I have an updated MR that meets these qualifications. Thank you for everyones feedback. |
|
Sounds good to me. I think the current names are safe, based on the current naming in the os package, but double checking or even (lightly) future proofing would be even better. |
|
https://go.googlesource.com/proposal/+/master/design/draft-iofs.md This just popped up on my radar today. It seems relevant to the discussion wrt being compatible with Go standard library modules & interfaces. I think it worth a read for anyone working on this project to take a look at and see what may be coming or even provide feedback to the Go project. @nixpanic @isi-lincoln |
|
I'm going to close this PR. On issue #327 I discussed the status of issue & pr with the submitter @isi-lincoln and this is not being actively worked on. I also mentioned that Go should be gaining new interfaces around file systems and it may be best to wait for that and make use of and test against that when it arrives. We can always reopen this and resume the work if needed. |
Cephfs Directory: export List and Names
Exports dir.list() and entries.names(). Becomes dir.List() and
entries.Names().
The current implementations for cephfs's list and names functions
are non-exported functions although they do not modify any private
fields. The functions would be of much more use to the community
as exported functions.
Signed-off-by: Lincoln Thurlow [email protected]