search

kennygrant / sanitize

Package sanitize provides functions for sanitizing text in golang strings.
BSD 3-Clause "New" or "Revised" License
334 stars 72 forks source link

Added WithOptions function to Name and Path #5

Closed nl5887 closed 9 years ago

nl5887 commented 10 years ago

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

eduncan911 commented 9 years ago

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 commented 9 years ago

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

eduncan911 commented 9 years ago

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.

nl5887 commented 9 years ago

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

kennygrant commented 9 years ago

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 commented 9 years ago

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

kennygrant commented 9 years ago

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.