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

Refactor internal functions to use opts map #88

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gfredericks
Copy link
Collaborator

Many internal functions had two or three arguments for customizing
behavior. Since we'd like to be able to add more customization in the
future, condensing these into an options map seems like a good move.

One subtle difference introduced is that the functions which previously
read a :date-format value from a map and used or to determine whether
to used the default-date-format, now use merge and so have different
behavior if the :date-format key is present but nil. I'm assuming
this will be fine.

Many internal functions had two or three arguments for customizing
behavior. Since we'd like to be able to add more customization in the
future, condensing these into an options map seems like a good move.

One subtle difference introduced is that the functions which previously
read a :date-format value from a map and used `or` to determine whether
to used the default-date-format, now use `merge` and so have different
behavior if the `:date-format` key is present but `nil`. I'm assuming
this will be fine.
@gfredericks
Copy link
Collaborator Author

This is of course w.r.t. #77.

@dakrone
Copy link
Owner

dakrone commented Oct 12, 2015

Okay, so having a child has unfortunately sucked up a lot of my time for open source projects, so let's improve the community!

@gfredericks I've added you as a contributor to the project, if you want to merge this (run the benchmarks first please!) that would be great! I'll continue to help with issues but I don't want to be a blocker :)

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.

2 participants