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

Cleanup #65

Merged
merged 6 commits into from
Jun 2, 2020
Merged

Cleanup #65

merged 6 commits into from
Jun 2, 2020

Conversation

niconoe
Copy link
Contributor

@niconoe niconoe commented May 26, 2020

Description

Some minor cleanup.

@coveralls
Copy link

coveralls commented May 26, 2020

Coverage Status

Coverage increased (+3.0%) to 80.366% when pulling 60b2017 on niconoe:cleanup into d82d021 on sckott:master.

@sckott sckott added this to the v0.5 milestone May 26, 2020
@sckott
Copy link
Collaborator

sckott commented May 26, 2020

Thanks. A few questions:

  • You've removed some of the import re lines. Why? e.g., in download.py re is used at https://github.com/sckott/pygbif/blob/master/pygbif/occurrences/download.py#L12
  • I'm not sure I understand some of the formatting changes (white space, quotes). In the commit d82d021 where I started using black I think I only ran it on the files in pygbif/. I'm not sure why there's additional white space/quote changes in those files in this PR? Maybe different black versions? I do appreciate the changes in other files though, setup.py, etc. that I didn't run black on

@niconoe
Copy link
Contributor Author

niconoe commented May 28, 2020

Thanks for your remarks!

I had to investigate a bit for the import re stuff in download.py. I initially removed it because I blindly followed my IDE (PyCharm) when it told me it was unused. But as you noticed, it's actually used at line 12. In fact, what happened is that it was cascade-imported from from ..gbifutils import *, and therefore not necessary to reimport in this file. This actually also happen with other packages in this file, that's why you were for example able to reference pygbif.__version__on line 176 or requests.* at different places. It's generally discouraged in Python to do from x import *, to avoid those namespace issues (namespace conflicts, or in the best case hard to understand situations like this). I therefore just pushed a fix for download.py that use only explicit imports. If you think we should do that, I should probably also do it for the rest of the package.

For the additional formatting:

  • black doesn't care about whitespace in docstrings, PyCharm was complaining about mixed tab/space indentation there, hence the change. For the quoting, I think it's just black doing its thing on files outside of the pygbif directory (where it was not run before).

import datetime
from requests import auth
import requests
import pygbif
Copy link
Collaborator

Choose a reason for hiding this comment

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

import pygbif ? I've not seen this in a python package before, importing the package that you're already in? curious what does that do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I don't like it much neither. I think a good explanation lies there: https://stackoverflow.com/questions/42862042/in-python-how-to-import-from-the-init-py-in-the-same-directory

I'll try to rearrange things a bit further so we don't need to import from init.py.

@sckott
Copy link
Collaborator

sckott commented May 28, 2020

Makes sense on the imports - had another question above about importing pygbif.^

@sckott
Copy link
Collaborator

sckott commented Jun 2, 2020

thanks!

@sckott sckott merged commit 441f5f6 into gbif:master Jun 2, 2020
@niconoe niconoe deleted the cleanup branch June 5, 2020 12:15
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.

3 participants