Replace print statements with logger#566
Conversation
|
I didn't update the notebooks which also contain |
|
Good question! I think the notebooks should probably stay the same. |
mfisher87
left a comment
mfisher87
left a comment
There was a problem hiding this comment.
Looks wonderful, thank you for taking this on! 🤩
A couple nitpicks :) What do you think?
|
Oo, also, what do you think of enabling ruff ruleset T20 to prevent us from re-introducing print statements? |
chuckwondo
left a comment
chuckwondo
left a comment
There was a problem hiding this comment.
Hi @botanical! Glad to see your PR! Just a couple of tweaks, please, but otherwise, great start to your getting involved here!
earthaccess/search.py
Outdated
| logger.info("Class components: \n") | ||
| logger.info([method for method in dir(self) if method.startswith("_") is False]) |
There was a problem hiding this comment.
These should actually remain print calls.
Good call. We would just want to disable this check for the lines in |
@botanical, if you can add this to the PR, that would be great. Note that at a minimum you'll have to pull in the latest changes from |
| try: | ||
| print(f"Testing S3 credentials for {daac['short-name']}") | ||
| logger.info(f"Testing S3 credentials for {daac['short-name']}") | ||
| credentials = earthaccess.get_s3_credentials(daac["short-name"]) | ||
| assertions.assertIsInstance(credentials, dict) | ||
| assertions.assertTrue("accessKeyId" in credentials) | ||
| except Exception as e: | ||
| print( | ||
| logger.error( | ||
| f"An error occured while trying to fetch S3 credentials for {daac['short-name']}: {e}" | ||
| ) |
There was a problem hiding this comment.
We shouldn't be logging messages from test functions. In fact, I suspect the try/except should be removed. If an exception occurs during the test, the test should fail. However, I don't know if this is an exception to that rule (no pun intended).
There was a problem hiding this comment.
botanical@64ec135 it seems intentional to me according to this commit 😯
There was a problem hiding this comment.
In fact, I suspect the try/except should be removed.
it seems intentional to me according to this commit 😯
It does still seem weird. But let's be intentional about the scope of this PR :) Can we take this to an issue / discussion?
There was a problem hiding this comment.
In fact, I suspect the try/except should be removed.
it seems intentional to me according to this commit 😯
It does still seem weird. But let's be intentional about the scope of this PR :) Can we take this to an issue / discussion?
Sounds good to me. @botanical, would you mind opening a separate issue for this? Also, would you open yet another issue about refactoring loops in tests to instead be parametrized tests (pytest.parametrize, and yes, that is how it's spelled, which is apparently a valid variant of parameterize).
There was a problem hiding this comment.
yes, that is how it's spelled, which is apparently a valid variant of parameterize
Gets me every time
There was a problem hiding this comment.
chuckwondo
left a comment
chuckwondo
left a comment
There was a problem hiding this comment.
@botanical, sorry, I made a number of individual comments rather than unified review comments.
Co-authored-by: Matt Fisher <[email protected]>
Co-authored-by: Matt Fisher <[email protected]>
Co-authored-by: Matt Fisher <[email protected]>
Co-authored-by: Chuck Daniels <[email protected]>
Co-authored-by: Chuck Daniels <[email protected]>
Co-authored-by: Chuck Daniels <[email protected]>
Co-authored-by: Chuck Daniels <[email protected]>
Co-authored-by: Chuck Daniels <[email protected]>
Co-authored-by: Chuck Daniels <[email protected]>
pyproject.toml
Outdated
| extend-select = ["I", "T20"] | ||
|
|
||
| [tool.ruff.lint.per-file-ignores] | ||
| "earthaccess/search.py" = ["T20"] |
There was a problem hiding this comment.
I do not know if this is necessarily what we want but I didn't know how to ignore that we have intentional print statements
earthaccess/search.py:312:9: T201
earthaccess/search.py:313:9: T201
There was a problem hiding this comment.
On those lines, you can append a comment like # noqa: T201 :) https://docs.astral.sh/ruff/linter/#error-suppression
chuckwondo
left a comment
chuckwondo
left a comment
There was a problem hiding this comment.
@botanical, thanks for the cleanup of the print calls and adding the ruff rule. All looks good, except that I see numerous changes that have already landed on main, so it seems perhaps that your attempt to merge things from main went awry somewhere. For example, .codespellignore already exists on main, but shows up in this PR as an addition (among several other existing changes on main). Can you perhaps rebase your changes onto main? Happy to jump on a call to help sort it out if wrangling the commits gets gnarly.
mfisher87
left a comment
mfisher87
left a comment
There was a problem hiding this comment.
Thanks so much @botanical 🤩
@chuckwondo (or anyone else) if you don't have any last thoughts I'll merge tomorrow!
@mfisher87, I'd like for us to use "Squash and merge" as a matter of habit. That's what I've been doing for PRs, as it keep the commit history much cleaner, unless there's an objection to that approach. |
This could be a whole 🔥-war discussion 🤣 and it's come up on slack before, but for Earthaccess I think squash-merging matches the current development/release workflow the best. I think we should disable all but 1 way to merge personally, so there's no question: |
|
I think we can call this mergeable, and squash it is! Thanks again @botanical 🙇 I don't have strong feelings about merge strategy, but I do feel strongly let's take it over here #348 :) |
4 participants
Addresses #511
📚 Documentation preview 📚: https://earthaccess--566.org.readthedocs.build/en/566/