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

accept strings in constructor #45

Merged
merged 3 commits into from
Jun 8, 2017
Merged

accept strings in constructor #45

merged 3 commits into from
Jun 8, 2017

Conversation

MaxBittker
Copy link
Contributor

@MaxBittker MaxBittker commented Jun 2, 2017

this will look for string-like inputs, and parse them correctly into patch sets, allowing the syntax in
#26

I'm happy to update examples/tests or otherwise improve this if you feel iffy about the implementation or behavior.👍

@MaxBittker
Copy link
Contributor Author

@matiasb hey there, there's no rush acting on this but I wanted to bring it to your attention in case it slipped through 👍

@matiasb
Copy link
Owner

matiasb commented Jun 7, 2017

In my queue! I'll be taking a look soon.
Thanks for the quick patch 👍

@MaxBittker
Copy link
Contributor Author

awesome, thanks. not high priority but I don't want to forget about it and I think it's helpful

@matiasb
Copy link
Owner

matiasb commented Jun 8, 2017

I'm still not convinced the init should guess the type before parsing, given there is an explicit classmethod for strings. In that way, it is responsibility of the caller to decide which constructor to use (and for example, if there is something that should be treated as string but it is not a string, he/she could make the decision instead of us guessing it wrong).

I'm merging the change, codewise looks good, but I think it may mutate before a next release. Thanks!

@matiasb matiasb merged commit 3bd0bf1 into matiasb:master Jun 8, 2017
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