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

Multi-account mutations #1

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@ivasic
Contributor

ivasic commented Jul 14, 2018

No description provided.

@djc

This comment has been minimized.

Show comment
Hide comment
@djc

djc Jul 13, 2018

Yeah, that's a good idea. Do you want to submit it as a PR? I actually thought it might make sense to make iban the first argument to mutations(), and possibly mandatory rather than optional.

djc commented on f593203 Jul 13, 2018

Yeah, that's a good idea. Do you want to submit it as a PR? I actually thought it might make sense to make iban the first argument to mutations(), and possibly mandatory rather than optional.

This comment has been minimized.

Show comment
Hide comment
@ivasic

ivasic Jul 14, 2018

Owner

sure, I'll send a PR. The change you proposed sounds absolutely reasonable, though it breaks backwards compatibility. If we were to do that, I'd definitely make iban mandatory otherwise existing (old) code would confuse iban for last_key causing runtime bugs. What are your thoughts @djc, makes sense to go ahead with it?

Owner

ivasic replied Jul 14, 2018

sure, I'll send a PR. The change you proposed sounds absolutely reasonable, though it breaks backwards compatibility. If we were to do that, I'd definitely make iban mandatory otherwise existing (old) code would confuse iban for last_key causing runtime bugs. What are your thoughts @djc, makes sense to go ahead with it?

This comment has been minimized.

Show comment
Hide comment
@djc

djc Jul 14, 2018

I'm not that worried about backwards compatibility at this stage, there probably aren't that many people using it. I'll also write up a changelog for the 0.2 release to describe the change. So yes, please go ahead (and make iban mandatory)!

djc replied Jul 14, 2018

I'm not that worried about backwards compatibility at this stage, there probably aren't that many people using it. I'll also write up a changelog for the 0.2 release to describe the change. So yes, please go ahead (and make iban mandatory)!

This comment has been minimized.

Show comment
Hide comment
@ivasic

ivasic Jul 14, 2018

Owner

Fair enough. Sending a PR then..

Owner

ivasic replied Jul 14, 2018

Fair enough. Sending a PR then..

@djc

This comment has been minimized.

Show comment
Hide comment
@djc

djc Jul 14, 2018

Owner

I've squashed your last two commits and pushed them as f593203 and 905987a. Thanks!

Owner

djc commented Jul 14, 2018

I've squashed your last two commits and pushed them as f593203 and 905987a. Thanks!

@djc djc closed this Jul 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment