config_checker update and linting for C408,C419,C405, C409, E721,F401,E722#560
config_checker update and linting for C408,C419,C405, C409, E721,F401,E722#560alex-rakowski wants to merge 47 commits intopy4dstem:devfrom
Conversation
merging in more ruff errors
sezelt
left a comment
There was a problem hiding this comment.
The majority of these changes are good, but at least one is wrong, and some are behavior changes that should be evaluated.
It would also be nice to clean up the TODOs before merging.
|
@sezelt I'm good with these edits but will wait for your approval as well. |
sezelt
left a comment
There was a problem hiding this comment.
It seems there are at least 7 remaining TODOs added by this PR. I'd prefer not to add too many of these, so if there are cases where it's possible to either deduce or test what errors should be captured we should do that. I also marked a few minor comments that would be good to clean up.
| try: | ||
| disks = disks / max(disks) | ||
| except: | ||
| # TODO work out what exception would go here |
There was a problem hiding this comment.
I'd guess this is a ZeroDivisionError, but I didn't want to guess I can remove the TODO if prefered.
py4DSTEM/io/filereaders/read_K2.py
Outdated
| Q_Nx = gtg.allTags[".SI Image Tags.Acquisition.Parameters.Detector.height"] | ||
| Q_Ny = gtg.allTags[".SI Image Tags.Acquisition.Parameters.Detector.width"] | ||
| except: | ||
| # TODO check this is the correct error type |
There was a problem hiding this comment.
I can remove as Exception will handle what we want, but I think it would be better to add the expected exception/s in the future, but I'm not familiar enough to check/test
| try: | ||
| N_coords = len(f[topgroup]["data/coordinates"].keys()) | ||
| except: | ||
| # TODO work out what exception will be raised ValueError, AttributeError, BS thinks KeyError |
There was a problem hiding this comment.
I can remove
There was a problem hiding this comment.
Actually, I'm going to leave this as a TODO I don't want to break this and I'm not familiar enough with this part to test
|
@sezelt, I've resolved most of the changes you requested. I would prefer to leave the remaining TODOs as I think we should address them next time they're worked on, but I don't have enough familiarity with the code to be confident in guessing or being able to test. |
|
Hey @sezelt @alex-rakowski - This is a large set of small edits which will affect the linting PR and likely modifies many similar pieces of code. I think it's best to finish / resolve this PR first, then rebase PR#563 once this is merged, before continuing to work on that PR. Otherwise we're going to run into a lot of conflicts and work duplication. Could you please also make a checklist of the kinds modifications made in this PR? (E.g. modifying exceptions, etc). I'd also request modifying the PR name. |
|
This PR does the following: Updates In addition, it lints for the following errors:
|
|
I've tried to sync this with |
|
It seems to not pass its own tests anymore, though, so something is off with the updated flake8 config... |
Getting rid of some of the bare exceptions. Adding the correct Error if I could work it out otherwise using
Exception.