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

Pub/Sub doc: missing definition for max_lease_duration in FlowControl #7043

Closed
anguillanneuf opened this issue Jan 3, 2019 · 7 comments · Fixed by #8293
Closed

Pub/Sub doc: missing definition for max_lease_duration in FlowControl #7043

anguillanneuf opened this issue Jan 3, 2019 · 7 comments · Fixed by #8293
Assignees
Labels
api: pubsub Issues related to the Pub/Sub API. type: docs Improvement to the documentation for an API.

Comments

@anguillanneuf
Copy link
Contributor

anguillanneuf commented Jan 3, 2019

The ref doc for max_lease_duration does not describe what it is or how it is used, especially about how it interacts with ack deadlines.

Given that this is an important config setting for long-running tasks, we may want to provide more documentation here by showing its default value (is it 2 hours), and explaining whether max lease duration restrains ack deadline modification or ack deadline modification overwrites max lease duration when one configures both flow control settings and ack deadlines.

@tseaver tseaver added api: pubsub Issues related to the Pub/Sub API. type: docs Improvement to the documentation for an API. labels Jan 4, 2019
@anguillanneuf
Copy link
Contributor Author

@tseaver How can we bring the default values (mentioned here) into the documentation?

@tseaver
Copy link
Contributor

tseaver commented Mar 19, 2019

@anguillanneuf FlowControl is a Python namedtuple, which provides no way to inject any additional semantic information into its docstring, or those of its members. We could generate a class derived from the named tuple, and put proto-a comment-based docstring, but we would still not be able to tweak docstrings for the members.

@anguillanneuf
Copy link
Contributor Author

anguillanneuf commented Mar 26, 2019

@crwilcox @sduskis Surfacing this for your attention.

@plamut
Copy link
Contributor

plamut commented Apr 19, 2019

Edit:
Oh, checked #2634 - are we talking about namedtuples whose declarations are autogenerated from something else? In that case, it's the generator that would have to grab the comments from "something else" and inject them into the generated namedtuple declaration ... oh well.
(the proposal below would persistently work just for the namedtuples manually defined by a human).


In Python 3.5+ the docstrings of namedtuples and their members can be updated:

from collections import namedtuple

FlowControl = namedtuple("FlowControl", ["foo", "bar"])                                                                                                                                                                                                                  
FlowControl.__doc__ = "These are the flow control settings."                                                                                                                                                                                                                 
FlowControl.foo.__doc__ = "The foo setting does X"                                                                                                                                                                                                                       
FlowControl.bar.__doc__ = "The bar setting does Y"  

With the modifications, invoking help(FlowControl) displays the following (abbreviated):

Help on class FlowControl in module __main__:

class FlowControl(builtins.tuple)
 |  FlowControl(foo, bar)
 |  
 |  These are the flow control settings.
 |  
 |  Method resolution order:
 |      ...
 |  
 |  ----------------------------------------------------------------------
 |  Data descriptors defined here:
 |  
 |  foo
 |      The foo setting does X
 |  
 |  bar
 |      The bar setting does Y
 |  
 | ...

No more default "Alias for field number X" descriptions of the members that end up in the linked docs. 🎉

In the code, we could optionally modify the namedtuples' docstrings if Python version >= 3.5, and generate the docs using one of the officially supported Python versions (which are, in fact, Python 3.5+).

Would that solve the issue, or am I missing something?

@sduskis
Copy link
Contributor

sduskis commented May 17, 2019

@plamut, It sounds like your solution would work. Please do it.

@plamut
Copy link
Contributor

plamut commented May 17, 2019

@sduskis The FlowControl settings class is auto-generated, thus the change needs to be done in the code generator (the ticket that Tres linked above).

If Luke (the current assignee) is too occupied with other work, I can help, although I would first have to familiarize myself with the gapic-generator lib.

@sduskis
Copy link
Contributor

sduskis commented May 17, 2019

oh. Thanks for the info @plamut. That sounds like a time intensive change, so I don't think that the effort is advisable in the near term. @anguillanneuf, I'm ok keeping this open, but this will be a very low priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. type: docs Improvement to the documentation for an API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants