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

Partial fix for #2841 to improve formatting. #2906

Merged
merged 1 commit into from
Apr 19, 2019

Conversation

dnowacki-usgs
Copy link
Contributor

Updates formatting to use .format() instead of % operator. Changed all instances of % to .format() and added test for using tuple as key, which errored using % operator.

@pep8speaks
Copy link

pep8speaks commented Apr 18, 2019

Hello @dnowacki-usgs! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-04-18 17:20:14 UTC

@shoyer
Copy link
Member

shoyer commented Apr 18, 2019

Thanks @dnowacki-usgs !

One minor note -- it's valid (since Python 2.7) to write string formatting with explicit positional indexes, e.g., you could just write '{} {}'.format('one', 'two') rather than '{0} {1}'.format('one', 'two').

I think it's usually a little cleaner to omit the numeric indices, unless you're reusing arguments multiple times or printing the names out of order. (But even in these cases it's usually nicer to use keyword names.)

@max-sixty
Copy link
Collaborator

Also come Py3.6, f strings 🙏

Updates formatting to use .format() instead of % operator. Changed all instances of % to .format() and added test for using tuple as key, which errored using % operator.

pep8

Remove positional indexes and fix !r formatting bug
@dnowacki-usgs
Copy link
Contributor Author

Thanks @shoyer. I think numeric indexes are clearer, though maybe not cleaner. Removed them and fixed the dask CI test I didn't catch the first time through. Hopefully everything passes now. 🤞

@shoyer
Copy link
Member

shoyer commented Apr 18, 2019

Looks great! You can ignore the failing doc build in the tests, that is unrelated.

If you like, feel free to give yourself a credit in whats-new.rst for fixing this. Otherwise I'll merge this in a day or two.

@dnowacki-usgs
Copy link
Contributor Author

Good to go as far as I'm concerned. I'm not even sure how I'd phrase these changes in whats-new, so let's go with it as-is.

@max-sixty
Copy link
Collaborator

OK, thanks @dnowacki-usgs !

@max-sixty max-sixty merged commit c8251e3 into pydata:master Apr 19, 2019
dcherian added a commit to yohai/xarray that referenced this pull request Apr 19, 2019
* master: (29 commits)
  Handle the character array dim name  (pydata#2896)
  Partial fix for pydata#2841 to improve formatting. (pydata#2906)
  docs: Move quick overview one level up (pydata#2890)
  Manually specify chunks in open_zarr (pydata#2530)
  Minor improvement of docstring for Dataset (pydata#2904)
  Fix minor typos in docstrings (pydata#2903)
  Added docs example for `xarray.Dataset.get()` (pydata#2894)
  Bugfix for docs build instructions (pydata#2897)
  Return correct count for scalar datetime64 arrays (pydata#2892)
  Indexing with an empty array (pydata#2883)
  BUG: Fix pydata#2864 by adding the missing vrt parameters (pydata#2865)
  Reduce length of cftime resample tests (pydata#2879)
  WIP: type annotations (pydata#2877)
  decreased pytest verbosity (pydata#2881)
  Fix mypy typing error in cftime_offsets.py (pydata#2878)
  update links to https (pydata#2872)
  revert to 0.12.2 dev
  0.12.1 release
  Various fixes for explicit Dataset.indexes (pydata#2858)
  Fix minor typo in docstring (pydata#2860)
  ...
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.

5 participants