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

black formatting & pyflakes alerts resolution #234

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

tgrandje
Copy link
Collaborator

I ran pytest and pyflakes on the whole code. I've also cleaned some comment (removed those which seemed unnecessary and kept those I had a doubt on), but that should be marginal.

There are also cases where I was completly sure of what to do, regarding some unused variables. Just to be sure, I flagged those in a different issue.

I suggest merging that PR if possible before merging anything else (I'll clean the current PR resolving github timeouts afterwards for instance).

Copy link
Collaborator

@tfardet tfardet left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I just have a few minor comments.
I also think it would be good to add a tutorial somewhere (readme.md) for contributors to install black and a pre-commit hook

@@ -33,8 +33,7 @@ def _add_insee_dep_region(df):
)
df = df.merge(dep, on="insee_reg", how="left")

except Exception as e:
# print(e)
except Exception:
pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be logged?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so, the other exceptions in the same file are already silenced... What do you think?

pynsee/geodata/_get_full_list_wfs.py Show resolved Hide resolved
pynsee/localdata/_get_insee_local_onegeo.py Outdated Show resolved Hide resolved
pynsee/macrodata/_download_idbank_list.py Outdated Show resolved Hide resolved
pynsee/macrodata/get_series_list.py Outdated Show resolved Hide resolved
pynsee/sirene/search_sirene.py Outdated Show resolved Hide resolved
@tgrandje
Copy link
Collaborator Author

I've just added pre-commits and a mention in the CONTRIBUTING.md. Please double-check this is working as expected, I'm in uncharted waters here 😉 !

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.

2 participants