8000 Ruff by jeanconn · Pull Request #300 · sot/mica · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Ruff #300

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 19 commits into from
Sep 26, 2024
Merged

Ruff #300

merged 19 commits into from
Sep 26, 2024

Conversation

jeanconn
Copy link
Contributor
@jeanconn jeanconn commented Aug 27, 2024 •
edited
Loading

Description

Ruff mica.

These changes include one bugfix, one API change, a few minor changes in the way errors are raised, and includes adding a with context manager for a few h5 file opens.

Interface impacts

  • bugfix - get_dark_cal_props in mica.archive.aca_dark.dark_cal now passes t_ccd_ref through to _get_dark_cal_image_props. This may not have any utility, but it looks like it was a bug to just drop it on the floor after taking it as a function option.

  • api change - function get_obs_temps in mica.report.report had a useless outdir argument that has been removed.

Testing

Unit tests

  • Linux
(ska3-masters) jeanconn-fido> git rev-parse HEAD
e2d50bb810488e176d0969b8ceef2940b226e56c
(ska3-masters) jeanconn-fido> pytest
======================================================== test session starts ========================================================
platform linux -- Python 3.11.8, pytest-7.4.4, pluggy-1.4.0
rootdir: /proj/sot/ska/jeanproj/git
configfile: pytest.ini
plugins: timeout-2.2.0, anyio-4.3.0
collected 108 items                                                                                                                 

mica/archive/tests/test_aca_dark_cal.py ..................                                                                    [ 16%]
mica/archive/tests/test_aca_hdr3.py .                                                                                         [ 17%]
mica/archive/tests/test_aca_l0.py ...                                                                                         [ 20%]
mica/archive/tests/test_asp_l1.py .......                                                                                     [ 26%]
mica/archive/tests/test_cda.py ..............................................                                                 [ 69%]
mica/archive/tests/test_obspar.py .                                                                                           [ 70%]
mica/report/tests/test_write_report.py .                                                                                      [ 71%]
mica/starcheck/tests/test_catalog_fetches.py ...............                                                                  [ 85%]
mica/stats/tests/test_acq_stats.py ...                                                                                        [ 87%]
mica/stats/tests/test_guide_stats.py ....                                                                                     [ 91%]
mica/vv/tests/test_vv.py .........                                                                                            [100%]

================================================== 108 passed in 780.21s (0:13:00)

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

No functional testing.

@jeanconn jeanconn marked this pull request as ready for review August 27, 2024 18:30
@jeanconn
Copy link
Contributor Author

I'll go through this and annotate my # noqa s with which rule and justification .

@jeanconn
Copy link
Contributor Author

I've at least gone through and annotated the noqa. I did not justify them - I think it is a good goal to get rid of them, but that maybe these are acceptable-enough on a first pass.

@taldcroft taldcroft requested a review from javierggt September 16, 2024 15:28
javierggt
javierggt reviewed Sep 25, 2024
View reviewed changes
Copy link
Contributor
@javierggt javierggt left a comment

Choose a reason for hiding this comment

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

There are two kind of comments that are repeated:

  • the idiom of raising from another exception, which I believe is not what one wants in most cases. In my experience, one either re-raises the same exception or raises from None, with some information from the deeper exception. When removing the warning, one should decide which is the case.
  • some unspecified noqa (although some might have been annotated in the last commit)

@jeanconn
Copy link
Contributor Author
jeanconn commented Sep 25, 2024 •
edited
Loading

And I'm sorry @javierggt, I thought I'd set PGH004 back on and cleared all the nonspecific "noqa". I'll do another pass. (it looks like all the nonspecific noqa had been handled but it wasn't turned back on in the ruff.toml)

@jeanconn
Copy link
Contributor Author

So yes, in general I figured that the exception handling raising "from err" was the most benign way to just do what the code was doing before to just satisfy ruff with no code improvements. It is a different question I think about if we want to use the ruff formatting as a moment to re-evaluate all of the exception handling.

@javierggt
Copy link
Contributor

I agree that raising from the previous exception carries the least impact. However, it is also the least preferred option and, realistically speaking, you will never come back to these. If it does not take a long time to figure out whether any of the other ways works, then now would be the time.

It does not take long to go through them to get it right. I just did and made comments. There is only one I think can be raise from the previous exception, and is the one about an unspecified exception in starcheck.

@jeanconn
Copy link
Contributor Author

Though I do disagree a bit with "realistically speaking, you will never come back to these." - I would like to come back to these - it is part of the reason I just wanted to start with Ruff fixes so we could start to clean up all of this code.

@javierggt
Copy link
Contributor

We can discuss what constitutes "useful" information any time.

@jeanconn jeanconn merged commit 693bfac into master Sep 26, 2024
2 checks passed
@jeanconn jeanconn deleted the ruff branch September 26, 2024 22:32
@jeanconn jeanconn mentioned this pull request Oct 4, 2024
Merged
2 tasks
@javierggt javierggt mentioned this pull request Oct 7, 2024
Closed
@javierggt javierggt mentioned this pull request Nov 7, 2024
Merged
@javierggt javierggt mentioned this pull request Nov 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javierggt javierggt javierggt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jeanconn @javierggt
0