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.

Sign up for GitHub

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

Jump to bottom

allow setting default :as for the whole client #299

Merged
merged 1 commit into from
Feb 15, 2018
Merged

allow setting default :as for the whole client #299

merged 1 commit into from
Feb 15, 2018
+37 −97

Conversation

grosser
Copy link
Contributor

@grosser grosser commented Feb 3, 2018

also passes all replies through format_response which will allow for easy addition of a new
:as type, I'm plaing to use JSON.parse(value, symbolize_names: true)

@cben
Copy link
Collaborator

cben commented Feb 3, 2018

Didn't review in detail but looks pretty good!
Some ideas, not sure if helping or overkill:

Consider making WatchStream too ask the client to format notices, so overriding format_response would affect it too. (May becomes better to split it into 3 methods rather than one returning 3 types depending on args.)

I've thought about letting :as param take an object, that's expected to have a method (or 3) implementing conversion of responses.
There would be a helper mapping :raw and :ros symbols into 2 such builtin objects; but any other object is possible.

@grosser grosser force-pushed the grosser/as_instance branch from b343d8c to 1f7a781 Compare February 8, 2018 05:26
@grosser
Copy link
Contributor Author

grosser commented Feb 8, 2018 •
edited
Loading

updated ... trying to keep this change as small as possible :D
looks good ?

should be a good base for refactoring into whatever else we want since all formatting is now centralized

@grosser
allow setting default :as for the whole client
c520eab 
also passes all replies through format_response which will allow for easy addition of a new
:as type, I'm plaing to use `JSON.parse(value, symbolize_names: true)`
@grosser grosser force-pushed the grosser/as_instance branch from 1f7a781 to c520eab Compare February 8, 2018 05:28
@cben
Copy link
Collaborator

cben commented Feb 11, 2018

LGTM 👍 @enoodle can you review as well?

@grosser can you document this in README?

@cben cben self-assigned this Feb 11, 2018
enoodle
enoodle approved these changes Feb 11, 2018
View reviewed changes
Copy link

@enoodle enoodle left a comment

Choose a reason for hiding this comment

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

LGTM, Maybe also update the README with this change?

@cben
Copy link
Collaborator

cben commented Feb 15, 2018

Merging; @grosser please either send followup to document this in README & CHANGELOG, or nag me to do it.

@cben cben merged commit 77aa4f0 into ManageIQ:master Feb 15, 2018
@grosser grosser mentioned this pull request Feb 16, 2018
Merged
This was referenced Mar 4, 2018
Closed
Closed
agrare added a commit to agrare/manageiq-providers-openshift that referenced this pull request Dec 4, 2018
@agrare
Kubeclient v3.1.0 removed Common::WatchClient
e202066 
ManageIQ/kubeclient#299 removed the
Kubeclient::Common::WatchNotice class, now watches just return
Kubeclient::Resource objects.
@agrare agrare mentioned this pull request Dec 4, 2018
Merged
@cben cben mentioned this pull request Mar 11, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enoodle enoodle enoodle approved these changes

Assignees

@cben cben

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@grosser @cben @enoodle