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

Fixes #254 move blocking call in db to sync #256

Merged
merged 1 commit into from Jun 14, 2019

Conversation

Projects
None yet
2 participants
@jonpither
Copy link
Member

commented Jun 13, 2019

I will review the docstrings etc, but wanted to set out this PR to show the direction, and get feedback.

@jonpither jonpither requested a review from hraberg Jun 13, 2019

@jonpither jonpither force-pushed the jp/254-move-blocking-to-sync branch from 61c6d54 to fe205f0 Jun 13, 2019

@hraberg
Copy link
Member

left a comment

Looks good, but two questions:

  1. We discussed renaming the method, did we decide against, or did we forget about it?
  2. I feel that timeout should be the last argument, for reasons I cannot fully motivate.
@jonpither

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

Agree with 2.

I can't remember what we finalised on with 1. I just decided to make the change, in the view that's easy to finalise the details after, so I'm happy to rename the method. I've just lost my strong opinion on it :-)

@hraberg

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

I don't mind the name as such, but usually want to avoid things clashing with clojure.core. That said, once someone has an opinion they can state it and use query replace, so we can park that.

@hraberg
Copy link
Member

left a comment

We want to change argument order from duration, date to date, duration.

@hraberg

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Also, to state the obvious, the name of this function is very old and not really related to this issue itself.

@jonpither

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

I think we'll stick with sync for now - we can always change in a subsequent release whilst we're in alpha.

Besides @malcolmsparks makes an eloquent case and impassioned defence for retaining sync.

@jonpither jonpither force-pushed the jp/254-move-blocking-to-sync branch 3 times, most recently from d7ee7c7 to a147e10 Jun 14, 2019

@jonpither jonpither merged commit 003531c into master Jun 14, 2019

@jonpither

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

I changed the arg order, good for merging, assuming we can rename sync at a later date (and we should discuss again).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.