-
Notifications
You must be signed in to change notification settings - Fork 7
Resolve pylint and other warnings
#146
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
base: develop
Are you sure you want to change the base?
Changes from 2 commits
d0935dc
087b974
d41d519
a33105a
50203dc
3bd028d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,9 +43,13 @@ def put(self, filename, data, **kwargs): | |
|
|
||
| def get_file_path(self, file_type, path_info): | ||
| if file_type == "preprocessed": | ||
| file_path = f'{S3_FILE_PATH}/{path_info["election_id"]}/data/{path_info["office"]}/data_{path_info["geographic_unit_type"]}.csv' | ||
| csv_file = f'data_{path_info["geographic_unit_type"]}.csv' | ||
| file_path = f'{S3_FILE_PATH}/{path_info["election_id"]}/data/{path_info["office"]}/{csv_file}' | ||
| elif file_type == "config": | ||
| file_path = f'{S3_FILE_PATH}/{path_info["election_id"]}/config/{path_info["election_id"]}' | ||
| else: | ||
| LOG.warning("Unknown file type %s", file_type) | ||
| file_path = None | ||
| return file_path | ||
|
|
||
|
|
||
|
|
@@ -128,7 +132,7 @@ def wait_for_versions(self, q): | |
| try: | ||
| future.result() | ||
| yield version, data | ||
| except Exception as e: | ||
| except Exception as e: # pylint: disable=broad-exception-caught | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is just to remove the tox error? We haven't really cared about those in the past, since we use our pre-commit hooks to make sure we are pep compliant.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case maybe we should disable Another option here would be to add more things to our list of
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any downside to disabling pylint? Do we use it in other live team repos? If we do, I think we just ignore the complaints.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll ask, but if they're using
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the answer is we should stop using tox and use pytest directly in the unit tests. Since there is no need to use two different linters
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmmmmmm...I'd argue in favor of using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lennybronner alright so I did some reading here and it seems like So yeah, let me know if you want me to remove |
||
| LOG.error(f"Error downloading {version['VersionId']}: {e}") | ||
|
|
||
| q.task_done() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vaguely remember us doing something previously to avoid this already. At least stopping it turning up in the logs. Am I misremembering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting 🤔 Maybe it was another spot where we can occasionally get divisions by zero? Without this, right now I get warnings here when running the unit tests 🤔 Curious 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you re-run the 2024 in the testbed (without writing anything to s3), do you see this warning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright so I ran this:
And the answer right now is no, BUT, I noticed that even though we're capturing warnings, they're not currently coming through in the logs. Having changed that, this actually generates a lot of warnings 😬
PR incoming 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jjcherian ! Hope you're doing well 😄 👋🏻
We noticed that these few lines can generate quite a few
invalid value encountered in divideanddivide by zero encountered in dividewarnings. If you clone thedevelopbranch and run, for example,you should see them. We were wondering what the best way to handle these warning is. Right now, in this PR, I've added code to ignore (and therefore silence) them, but do you think there's a better way to resolve these? 🤔
Thanks in advance 😄