Skip to content
This repository has been archived by the owner on Mar 20, 2018. It is now read-only.

Rename and refactor CallSettings #112

Open
jmuk opened this issue Jun 22, 2016 · 10 comments
Open

Rename and refactor CallSettings #112

jmuk opened this issue Jun 22, 2016 · 10 comments
Assignees
Labels

Comments

@jmuk
Copy link
Contributor

jmuk commented Jun 22, 2016

Currently we have CallSettings and CallOptions, and it's hard to say their roles and the differences.

Actually CallSettings is the settings (or configurations) for an API call (or a method), such as the page descriptors or bundling descriptors, retry parameters.

On the other hand, CallOptions is the optional data to modify the behavior of individual invocation of the methods.

To me,

  • CallSettings isn't a very clear name. 'Call-' prefix sounds like per-call (per-invocation) thing. Maybe, MethodSettings?
  • the 'merge' method to create a new call settings doesn't look a good design after the redesign of create_api_call. I feel like CallSettings (or MethodSettings) should hold the default CallOptions instance, and CallOptions instance should have the merge() method to create the actual options for the invocation.

Thoughts?

@jmuk
Copy link
Contributor Author

jmuk commented Jun 22, 2016

@geigerj @bjwatson @tbetbetbe

@jmuk
Copy link
Contributor Author

jmuk commented Jun 22, 2016

I think this would be a good task to starting up for @landrito. This is for gax-python, but the same discussion must be applied to gax-ruby/nodejs as well (because we ported the python design to them).

@garrettjonesgoogle
Copy link
Member

In PHP, we just created CallSettings. I had this same thought about the confusion of CallOptions and CallSettings. In a case where you want both as classes, I might suggest DefaultCallSettings and CallSettings.

If you had MethodSettings + CallSettings, I don't think the relationship would be clear.

@jmuk
Copy link
Contributor Author

jmuk commented Jun 22, 2016

I'm afraid that DefaultCallSettings with CallSettings wouldn't suggest the right relationship between them. It sounds like DefaultCallSettings is a subclass of CallSettings -- but they are not.

My suggestion is to have MethodSettings (which holds the static properties of methods, such as page-streaming) with CallOptions (which holds updatable values), different prefixes and suffixes, and that would make things clearer.

I want to change the prefix because (to me) 'Call-' prefix seems to specify something for per-invocation. But the values for CallSettings are to determine the structure of the API call before the actual invocation happens (such as page-streaming or bundling or not).

@jmuk
Copy link
Contributor Author

jmuk commented Jun 22, 2016

MethodProperties might be an idea btw.

@garrettjonesgoogle
Copy link
Member

I currently like ApiCallableSettings.

@jmuk
Copy link
Contributor Author

jmuk commented Jul 21, 2016

Idea from @bjwatson: simply consolidate them into a single class, and expose the APIs (setter/getter) to operate internal data?

@jmuk
Copy link
Contributor Author

jmuk commented Jul 21, 2016

CallSettings --> _CallOptions (private class)

@bjwatson
Copy link
Contributor

bjwatson commented Aug 9, 2016

The smallest possible action we can take for this quarter is to prepend CallSettings with an underscore: CallSettings -> _CallSettings. That way no one will consider this to be a breakable surface.

This also affects Ruby.

@bjwatson bjwatson added P0 P1 and removed P0 labels Aug 9, 2016
jmuk added a commit to jmuk/gax-ruby that referenced this issue Aug 18, 2016
In Ruby, a 'private constant' is a constant which will have
to be accessed without prefix. So all entties inside of
Google::Gax can still access it as-is, but outside the module
it can't be accessed as Google::Gax::CallSettings.

This affects the tests, so this patch introduced a renaming
to toplevel ::CallSettings in a way to avoid violating
access of private constant.

updates googleapis/gax-python#112
jmuk added a commit to googleapis/gax-ruby that referenced this issue Aug 18, 2016
In Ruby, a 'private constant' is a constant which will have
to be accessed without prefix. So all entties inside of
Google::Gax can still access it as-is, but outside the module
it can't be accessed as Google::Gax::CallSettings.

This affects the tests, so this patch introduced a renaming
to toplevel ::CallSettings in a way to avoid violating
access of private constant.

updates googleapis/gax-python#112
@geigerj geigerj reopened this Aug 18, 2016
@geigerj
Copy link
Contributor

geigerj commented Aug 18, 2016

Actually, re-open because we still may wish to evaluate a future renaming. But because it is no longer exposed to users, downgrade priority to P3.

@geigerj geigerj added P3 and removed P1 labels Aug 18, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants