-
Notifications
You must be signed in to change notification settings - Fork 77
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
add: fn{parse, acceptedFileTypes}; #99
base: master
Are you sure you want to change the base?
Conversation
Would you consider maybe adding a clearer title and a little description of what this PR does and what its aim is. Commit messages in this project aim at explaining what the change does, not how the code has changed. This hardly gives an idea what feature you add or what problem you solve except if people go look at the code. Thank you! |
Sorry. The Parse function takes a file path and produces a metadata for readonly use-case the AcceptedFileTypes function allows users to iterate over accepted file types, say when creating a tree-walking file organizer, to avoid trying to check any files that may not match. Could use magic numbers for that, but not sure the time tradeoff is worthwile for my use case. Ultimately, just sharing them because I wanted to keep using them without continuing to use a fork, but if you don't want them that's fair too. edit |
Thanks for adding some more info. I'm actually not the developer of this library and not even that good at Go. I'm following this repo however since I'm quite invested in its maintenance and I hope someone will at some point contribute writing functionality to it! I expect that @dhowden will respond to your PR in due time :) |
@bertvandepoel what kind of functionality are you looking for? can maybe help if not too big |
I'm afraid I'm hoping for #15 (aka #71). So not really a small thing to achieve I'm afraid. But let's discuss that inside those issues and not derail this PR :) |
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.
Hey! Thanks for the change.
Understand completely the motivation to have the supported file types exported as a list. Having Parse
(not sure of the naming here - is there precedent?) as a convenience makes sense too.
) | ||
|
||
// ErrNoTagsFound is the error returned by ReadFrom when the metadata format | ||
// cannot be identified. | ||
var ErrNoTagsFound = errors.New("no tags found") | ||
|
||
// Supported file types. | ||
func AcceptedFileTypes() []FileType { |
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.
SupportedFileTypes would seem like a better name here.
// Supported file types. | ||
func AcceptedFileTypes() []FileType { | ||
return []FileType{ | ||
FileType(strings.ToLower(string(MP3))), |
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.
The FileType
type is constructing an enumeration of the possible values (as far as Go can do with enums!), this should just return the values as-is without transforming them.
// for readonly operations | ||
func Parse(path string) (Metadata, error) { | ||
f, err := os.Open(path) | ||
if err == nil { |
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.
Convention here is to check for err != nil
and handle that first.
} | ||
} | ||
|
||
// Parse metadata from the file at the given path |
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 would replicate the doc from ReadFrom, adding that it's reading from a file.
No description provided.