Skip to content
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

Added WithOptions function to Name and Path #5

Closed
wants to merge 1 commit into from

Conversation

nl5887
Copy link

@nl5887 nl5887 commented Nov 17, 2014

Added WithOptions function to Name and Path to allow optional lower and uppercasing and being compatible with old function.

@eduncan911
Copy link
Contributor

If you are adding an options, I highly prefer the functional options pattern:

http://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis

The main point is that you can add additional functionality, and options, to the package as it evolves and it won't break backwards compatibility. It also allows you to setup defaults.

I've been using this in all of my packages ever since.

@nl5887
Copy link
Author

nl5887 commented Feb 20, 2015

Check, implemented the functional options pattern. Agree that using this pattern is more future proof.

@eduncan911
Copy link
Contributor

I'm not the author of this package. Just a by standard looking around as I have other PRs. :-)

Awesome u went the functional options route.

With that said, I personally like clean single-commits to merge I to my repos. Maybe do:

git fetch
git rebase -i origin/master 
git push -f origin [your-branch-name]

Pick your first one, leaving it "pick", and then replace "pick" with "s" to squash all other commits to a single commit.

Added WithOptions function to Name and Path, to allow optional lower and
uppercasing and being compatible with old function using the functional
options pattern. Added tests for NameWithOption
@nl5887
Copy link
Author

nl5887 commented Feb 22, 2015

Rebased on master. This is a lot cleaner indeed. Thanks a lot for your comments, this improves my pull request quality skills :-)

@kennygrant
Copy link
Owner

I'd like some time to think about this, I will get back to you when I have, I'm not entirely convinced about the functional style of options as it makes them less explicit.

@nl5887
Copy link
Author

nl5887 commented Feb 22, 2015

Ok, let me know when you've made a decision. I'll be happy to implement it either way.

@kennygrant
Copy link
Owner

Sorry, I'm not keen on adding WithOptions functions for these because of the complexity added to simple functions. Path and Name are intended for use when saving user-supplied filenames to disk, but not for use reproducing user-supplied filenames as they are and potentially comparing with the original name later - the transform is deliberately lossy and restrictive and is definitely one-way.

Looking at the reporting issue, I believe the user would also expect other idiosyncrasies of their filenames to come through unchanged as well as case (e.g. use of other characters like & or $) - you might be best to simply take path.Clean(path.Base(fileName)) rather than importing this package.

@kennygrant kennygrant closed this Feb 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants