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

Added pre-commit config and ran entire code base through it #78

Merged
merged 5 commits into from
Aug 11, 2022

Conversation

shreyb
Copy link
Collaborator

@shreyb shreyb commented Aug 5, 2022

Added pre-commit config with black, pylint, and mypy tests, along with a few other hooks.

Lots and LOTS of changes, so reviewers, please take your time as needed.

@shreyb shreyb marked this pull request as draft August 5, 2022 03:51
@shreyb
Copy link
Collaborator Author

shreyb commented Aug 5, 2022

Moved to draft status because of a couple of testing issues.

@shreyb
Copy link
Collaborator Author

shreyb commented Aug 5, 2022

Fixed the testing issues, but I realized a couple of my unit tests fail because of fife_launch and POMS things. Will discuss with @marcmengel before moving this to Ready for Review.

@shreyb
Copy link
Collaborator Author

shreyb commented Aug 5, 2022

Marc agreed that /grid/fermiapp-dependent code might come out in the future anyhow (we're discussing offline). For now, I mounted /grid/fermapp to my dev node, and the package unit tests work. So I'm going to move this to "ready".

@shreyb shreyb marked this pull request as ready for review August 5, 2022 14:49
@shreyb
Copy link
Collaborator Author

shreyb commented Aug 5, 2022

Reviewers: Please note that most of the changes are whitespace/EOF changes.

Copy link
Contributor

@marcmengel marcmengel left a comment

Choose a reason for hiding this comment

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

Looks good. Only thing I would have done differently is just made all the units conversion tables in utils.py be Dict[str,float] with a few strategic .0's added.

@shreyb
Copy link
Collaborator Author

shreyb commented Aug 5, 2022

Thanks Marc - I'll do what you said and change the units conversion tables before merging this then (and I'll rerun tests of course).

@shreyb
Copy link
Collaborator Author

shreyb commented Aug 5, 2022

Made Marc's suggested changes. @retzkek, @goodenou do either of you have any comments before I merge this in (next week, naturally)?

Copy link
Collaborator

@goodenou goodenou left a comment

Choose a reason for hiding this comment

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

I reviewed the "tests" directory. Changes all look good.

shreyb added 2 commits August 11, 2022 02:04
In lib/fake_ifdh, we were masking a bug where if you say "return '' and int('')>0",
for example, we'd expect to get a ValueError from the second conditional, but instead
an empty string is returned.  I fixed the function so that we explicitly only do the
second check, and reraise the ValueError.

Also fixed a bug where we don't try to read the tokenfile if it doesn't exist, since
os.popen(anything_with_nonexistent_file) will simply return an empty string if we save
its results into a file-like object.
Copy link
Contributor

@retzkek retzkek left a comment

Choose a reason for hiding this comment

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

LGTM

@shreyb
Copy link
Collaborator Author

shreyb commented Aug 11, 2022

Thanks to all three of you for reviewing this monumental PR. Please go ahead and set up pre-commit (https://pre-commit.com/), and if you need help with it, let me know.

@shreyb shreyb merged commit 1845785 into fermitools:master Aug 11, 2022
@shreyb shreyb linked an issue Aug 23, 2022 that may be closed by this pull request
@shreyb shreyb linked an issue Aug 26, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants