Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixes #482: async mgr "reuseable -> reusable" #483

Merged
merged 1 commit into from
Apr 12, 2019

Conversation

ptuckey
Copy link
Contributor

@ptuckey ptuckey commented Apr 10, 2019

This addresses a naming inconsistency in conn mgr functions.

@ptuckey
Copy link
Contributor Author

ptuckey commented Apr 10, 2019

As per CONTRIBUTING, I ran:

lein all test :all

Initially I received a timeout error, but a subsequent attempt passed, so I'm guessing it's unrelated.


I wasn't sure if marking the existing function name as deprecated was necessary/appropriate.

I found a comment here on the subject wrt Clojure:
https://groups.google.com/forum/#!topic/clojure/0HoNstam09g

But, I figured I'd start with this PR as is, and see what folks thought.

@@ -181,7 +181,8 @@
(let [regular (conn-mgr/make-regular-conn-manager {})
regular-reusable (conn-mgr/make-reusable-conn-manager {})
async (conn-mgr/make-regular-async-conn-manager {})
async-reusable (conn-mgr/make-reuseable-async-conn-manager {})]
async-reusable (conn-mgr/make-reusable-async-conn-manager {})
async-reuseable (conn-mgr/make-reuseable-async-conn-manager {})]
Copy link

@danielcompton danielcompton Apr 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should either be a second assertion below for the async-reuseable , or only the correctly spelled one should be tested here.

This addresses a naming inconsistency in conn mgr functions.
@ptuckey ptuckey force-pushed the 482_make-reusable-async-wrapper branch from 01b471e to 4af6c33 Compare April 11, 2019 14:47
@dakrone dakrone merged commit b88084a into dakrone:3.x Apr 12, 2019
@dakrone
Copy link
Owner

dakrone commented Apr 12, 2019

Merged this, thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants