search

civisanalytics / civis-python

Civis API Python Client
BSD 3-Clause "New" or "Revised" License
34 stars 26 forks source link

[SERD-1446] ENH Added run_template function #318

Closed arthir closed 5 years ago

arthir commented 5 years ago

Mimics the civis-R run_template function.

stephen-hoover commented 5 years ago

Almost forgot -- this would also need an entry in the changelog.

arthir commented 5 years ago

Thanks for adding this! High level comments -- Keith may have other opinions, but I think that civis.utils._jobs is the most natural place to put this new function. It's not IO related. It should also take an APIClient object as its last parameter, defaulting to None. The other helper functions use this pattern, and it does make testing much easier.

Moved the function; also added the client as the last parameter.

keithing commented 5 years ago

@arthir and I talked about this online, but we agree with @stephen-hoover that civis.utils._jobs is the right place for this function. As a reminder, once we move that we'll need to add this function to __all__ in civis.utils.__init__ in order to import it with civis.utils.run_template.

arthir commented 5 years ago

Almost forgot -- this would also need an entry in the changelog.

Added to the changelog (I'm not sure if there's a specific style to follow).

jacksonllee commented 5 years ago

How is the user going to discover this awesome new function? Would it be documented in the Sphinx docs? Relatedly, I could also see the run_job function is arguably undocumented (though subtly mentioned). Would both run_X functions deserve a small section on the User Guide page, for example?

arthir commented 5 years ago

How is the user going to discover this awesome new function? Would it be documented in the Sphinx docs? Relatedly, I could also see the run_job function is arguably undocumented (though subtly mentioned). Would both run_X functions deserve a small section on the User Guide page, for example?

I can add (both) to the docs. Thanks for pointing it out.

arthir commented 5 years ago

Code looks good to me. Before merging would you mind

  1. Adding a test
  2. Addressing @stephen-hoover's comment about examples in the docstring.
arthir commented 5 years ago

@keithing Tests added (and work!) Also docs.

arthir commented 5 years ago

@keithing @stephen-hoover I think I need to re-request reviews after that last comment change.