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

Ease extension of Slugify class #44

Merged
merged 1 commit into from
Sep 24, 2014
Merged

Ease extension of Slugify class #44

merged 1 commit into from
Sep 24, 2014

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Sep 23, 2014

I made a few minor changes.

Changed the visibility of rules and rulesets properties to protected to allow them to be overriden or used in child classes while extending Slugify.
Maybe using a getter for the rules property would do the same job, but this was my approach.

Added a third optional param to the slufigy method with the regular expression to be applied to the string.
I made this because I needed to allow other characters in the slug, like dots for filenames. I didn't want My Cool File.zip to become my-cool-file-zip but my-cool-file.zip.
I wasn't sure if that argument should be set in the constructor or in this method, but I think this is good enough.

I was thinking also on giving the option to define if the string should be lowered or not, but I'm not sure what's the best way to do that.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 6e365df on acelaya:feature/protected-properties into 56b8981 on cocur:master.

@florianeckerstorfer
Copy link
Member

First you should split this up into two Pull Requests. These are completely different things.

I will merge the PR to change the visibility of the properties, but I don't think it is useful to change the interface of slugify(). If you want file names in the way you want them, you should remove the extension before passing the name to slugify and add it back later.

@acelaya
Copy link
Contributor Author

acelaya commented Sep 24, 2014

You are right, this should be split into two pull requests. I'll do it right away.
About changing the interface specification (which I indeed forget, I just updated the concrete class), probably making properties protected will ease customize the regular expression by extending the Slugify class.

@acelaya
Copy link
Contributor Author

acelaya commented Sep 24, 2014

Done, just removed the last commit and pushed it again (with --force)

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 0230408 on acelaya:feature/protected-properties into 56b8981 on cocur:master.

florianeckerstorfer pushed a commit that referenced this pull request Sep 24, 2014
@florianeckerstorfer florianeckerstorfer merged commit 9da3415 into cocur:master Sep 24, 2014
@florianeckerstorfer
Copy link
Member

Regarding the regular expression. If there is need to modify it, there should be another way than supplying another parameter to slugify(). I would suggest making the regular expression a property. Either we supply a setter method or we extend the Slugify class if we need a different regexp.

@acelaya
Copy link
Contributor Author

acelaya commented Sep 24, 2014

Yes, I like the idea of using a property. A protected property could be overriden, but maybe a setter would be good too.
I'm even thinking on providing a constructor with the regular expression as an optional argument with the current regular expression as the default value.
If you like the idea I'll make another pull request.

@florianeckerstorfer
Copy link
Member

Yes, please make another PR. Thanks.

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