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

Fix incorrect docstring in automate.py regarding date_ranges #298

Closed
wants to merge 1 commit into from
Closed

Conversation

jdossgollin
Copy link
Contributor

As comment suggested, this fixes a documentation bug pointed out in #295

@@ -384,10 +384,13 @@ def submit_mult_calcs(calc_suite_specs, exec_options=None):
The region(s) over which any regional reductions will be performed.
If 'all', use all regions in the ``regions`` attribute of each
``Proj``.
date_ranges : 'default' or tuple of datetime.datetime objects
date_ranges : 'default' or a list of tuples of datetime.datetime objects
Copy link
Contributor

Choose a reason for hiding this comment

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

E501 line too long (80 > 79 characters)

The range of dates (inclusive) over which to perform calculations.
If 'default', use the ``default_start_date`` and
``default_end_date`` attribute of each ``Run``.
Else provide a list, each containing a tuple of start and end dates
such as ``date_ranges=[(sdate, edate)]``, where ``sdate`` and ``edate``
Copy link
Contributor

Choose a reason for hiding this comment

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

E501 line too long (83 > 79 characters)

@jdossgollin
Copy link
Contributor Author

@spencerkclark
Do I need @pytest.mark.xfail here as suggested here?

@spencerkclark
Copy link
Collaborator

Do I need @pytest.mark.xfail here as suggested here?

Thanks @jdossgollin, we just need that in one PR -- it could be this one; it could your other one. Once that fix is merged the Appveyor and Travis checks will succeed. Stickler-CI checks for formatting issues; in this case it found a couple lines that are too long by PEP8 standards. If you shorten each of them a bit, that check will succeed too.

The range of dates (inclusive) over which to perform calculations.
If 'default', use the ``default_start_date`` and
``default_end_date`` attribute of each ``Run``.
Else provide a list, each containing a tuple of start and end dates
Copy link
Collaborator

Choose a reason for hiding this comment

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

No reason for you to know this :), but in the master (i.e. unreleased) version of aospy we also support partial datetime strings (e.g. '0001'), np.datetime64, or cftime.datetime objects as well. It would be great if you could update the docstring here to reflect that too. Maybe something along the lines of:

Else provide a list of tuples, each containing a pair of start and end dates,
such as ``date_ranges=[(start, end)]`` where ``start`` and ``end`` are 
each ``datetime.datetime`` objects, partial datetime strings (e.g. '0001'),
``np.datetime64`` objects, or ``cftime.datetime`` objects.

@spencerahill
Copy link
Owner

@jdossgollin thanks for this. A couple tips for PRs:

  • It's helpful to make the PR name a bit more descriptive. I've renamed this one as such.
  • It's helpful to have the initial PR message start with "Closes ###", where "###" is the Issue number that the PR is meant to address. Then Github will automatically close the Issue once the PR gets merged.

@spencerahill spencerahill changed the title Update automate.py Fix incorrect docstring in automate.py regarding date_ranges Nov 10, 2018
@jdossgollin jdossgollin deleted the patch-2 branch November 10, 2018 19:43
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

4 participants