Skip to content

Conversation

@Bear-Foot
Copy link
Contributor

No description provided.

@Bear-Foot Bear-Foot changed the title WIP adressing #126 Add clearHistoryTypes (#126) Oct 29, 2016
@omnidan
Copy link
Owner

omnidan commented Nov 4, 2016

Thanks for the PR, @Bear-Foot! What do you think about allowing arrays in clearHistoryType instead of making a new field? That way we wouldn't bloat the API too much.

@Bear-Foot
Copy link
Contributor Author

Here we go. I didn't want to break the switch case, but computed case values feel weird, what do you think ?

@omnidan
Copy link
Owner

omnidan commented Nov 21, 2016

@Bear-Foot sorry for the late response. It looks fine, but if you want to avoid using computed values in the case statement, you can always turn it into an array if it's not specified as such (when parsing the config). Then you always have the same check if the type is in the array. I think that would be a bit simpler.

@omnidan
Copy link
Owner

omnidan commented Dec 20, 2016

@Bear-Foot I haven't heard back from you yet - do you still want to change anything in regards to my comment above, or is this PR ready to merge now? Please let me know 😄

@Bear-Foot
Copy link
Contributor Author

Hey sorry I was really busy and didn't free some time for this, I'll do this by the end of the week :)

@omnidan omnidan merged commit f0d4060 into omnidan:master Dec 26, 2016
@omnidan
Copy link
Owner

omnidan commented Dec 26, 2016

Thanks a lot @Bear-Foot! I published beta9-4 with this change 👌

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