sloum
/
mailcap
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Some thoughts #1

New Issue
Closed
opened 2019-07-05 18:22:50 +00:00 by jboverf · 3 comments
jboverf commented 2019-07-05 18:22:50 +00:00
Copy Link

Hi! Finally got a chance to take a look. My main thoughts are related to the types exposed by the package. I think the API surface area is larger than is needed. I'd suggest removing the Cap and Entries types and renaming the Mailcap type to "DB". As it stands now, the Mailcap type looks like "Mailcap.Mailcap" to other packages. It's really the database of mailcaps - "mailcap.DB". The other two types aren't really needed unless you think you'll want to add methods to them later.

Also I like the convenience method for just executing the command, but rather than a low level one returning a string, how about one that returns an exec.Command instead?

Anyway those are all pretty minor - I like it!

Hi! Finally got a chance to take a look. My main thoughts are related to the types exposed by the package. I think the API surface area is larger than is needed. I'd suggest removing the Cap and Entries types and renaming the Mailcap type to "DB". As it stands now, the Mailcap type looks like "Mailcap.Mailcap" to other packages. It's really the database of mailcaps - "mailcap.DB". The other two types aren't really needed unless you think you'll want to add methods to them later. Also I like the convenience method for just executing the command, but rather than a low level one returning a string, how about one that returns an exec.Command instead? Anyway those are all pretty minor - I like it!
sloum commented 2019-07-06 20:43:16 +00:00
Owner
Copy Link

Thanks so much for taking a look!

I have waffled back and forth about what to expose and what not to expose. I think I agree with you about removing the exposed elements you mentioned. I tend to fall on the side of: "meh, people will figure out how to work with it, why limit them?"... but it definitely feels more right/polished to not have those exposed. I'll update.

I think your idea to change Caps to DB is spot on. I will do so.

I don't think I will need to add methods to the Entries type. I mostly made it because I wasn't sure the syntax to make a slice as the value side of a map and it was late so I just made a type and used it for that.

I had never thought of returning an exec.Command instance. It makes sense. Though it does change up the way I had envisioned things working. Are talking about the GetCommand method (which returns a string)? Or the Execute method (which does not return anything)? If the execute method: that makes sense and is more flexible for directing stdin/out/err if a little more verbose once called. Sounds like it would work well though and be a good usable way of doing things. I'll set it up :)

Thanks so much for taking a look! I have waffled back and forth about what to expose and what not to expose. I think I agree with you about removing the exposed elements you mentioned. I tend to fall on the side of: "meh, people will figure out how to work with it, why limit them?"... but it definitely feels more right/polished to not have those exposed. I'll update. I think your idea to change Caps to DB is spot on. I will do so. I don't think I will need to add methods to the Entries type. I mostly made it because I wasn't sure the syntax to make a slice as the value side of a map and it was late so I just made a type and used it for that. I had never thought of returning an exec.Command instance. It makes sense. Though it does change up the way I had envisioned things working. Are talking about the `GetCommand` method (which returns a string)? Or the `Execute` method (which does not return anything)? If the execute method: that makes sense and is more flexible for directing stdin/out/err if a little more verbose once called. Sounds like it would work well though and be a good usable way of doing things. I'll set it up :)
sloum commented 2019-07-06 21:45:56 +00:00
Owner
Copy Link

I updated the things discussed above. If you get another chance to do a super quick runthrough to see if I missed anything that would be awesome! I really appreciate the help!

I updated the things discussed above. If you get another chance to do a super quick runthrough to see if I missed anything that would be awesome! I really appreciate the help!
jboverf commented 2019-07-07 05:51:55 +00:00
Author
Copy Link

Looks good to me! You had it right on the exec.Cmd thing - sorry I wasn't too clear on that!

Looks good to me! You had it right on the exec.Cmd thing - sorry I wasn't too clear on that!
sloum closed this issue 2019-11-23 18:36:11 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
master
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: sloum/mailcap#1
No description provided.
Delete Branch "%!s(<nil>)"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?