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

Changing to connect everytime #25

Merged
merged 1 commit into from
Mar 23, 2020
Merged

Conversation

TimoKramer
Copy link
Contributor

Since we had problems using this lib in bob, we decided to change the
behaviour to connect everytime you call invoke. The connect function is
now deprecated and only returns a map to avoid breaking the api. You
still are able to use client like before but it is discouraged now
to use connect and only using invoke is also sufficient.

Refers #20

@TimoKramer
Copy link
Contributor Author

should the client-function be integrated into the invoke-function?

@lispyclouds
Copy link
Collaborator

lispyclouds commented Mar 22, 2020

No its needed for fns like doc and ops and it carries the path context, so it has use outside.

src/clj_docker_client/core.clj Outdated Show resolved Hide resolved
project.clj Outdated Show resolved Hide resolved
@TimoKramer
Copy link
Contributor Author

requested changes applied

@TimoKramer
Copy link
Contributor Author

will adapt the documentation now

@lispyclouds
Copy link
Collaborator

(.callTimeout (timeout-from (:call-timeout timeouts))))]
{:client (.build builder+opts)
:socket socket}))
(:import (okhttp3 OkHttpClient$Builder)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This import can be removed?

@TimoKramer TimoKramer force-pushed the master branch 2 times, most recently from 8aa17de to bc675f8 Compare March 22, 2020 19:13
@TimoKramer
Copy link
Contributor Author

panic! and connect* are now in requests-ns. maybe panic should be moved back to core?! I wasn't sure...

@lispyclouds
Copy link
Collaborator

I think panic! should be moved back as its supposed to handle public APIs

Since we had problems using this lib in bob, we decided to change the
behaviour to connect everytime you call invoke. The connect function is
now deprecated and only returns a map to avoid breaking the api. You
still are able to use client like before but it is discouraged now
to use connect and only using invoke is also sufficient.

I moved the connect-function to the requests namespace because
it is not part of the official api but is used with the fetch-function
and therefore should reside with it. The panic! function has to be
in the requests namespace as well because otherwise we would have a
cyclic dependency. The documentation is changed accordingly and fetch
needs to call the for now unofficial function connect*.

Refers into-docker#20
@TimoKramer
Copy link
Contributor Author

alright. added the uri-check and inlined the timeouts for the deprecated connect. I need the panic! in the request-ns because using it as an import from core does not work because of cyclic dependency. we could make a util-ns?!

@lispyclouds
Copy link
Collaborator

Ah yes, we can go ahead with it being in requests ns.

@TimoKramer
Copy link
Contributor Author

from my point of view it should be fine to merge and release the new version.

@lispyclouds lispyclouds merged commit beda041 into into-docker:master Mar 23, 2020
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.

None yet

2 participants