Skip to content

Conversation

@alex-rakowski
Copy link
Collaborator

Getting rid of some of the bare exceptions. Adding the correct Error if I could work it out otherwise using Exception.

@alex-rakowski alex-rakowski changed the title Clearing up some bare exceptions Clearing up minor issues Nov 10, 2023
@alex-rakowski
Copy link
Collaborator Author

alex-rakowski commented Nov 10, 2023

Copy link
Member

@sezelt sezelt left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Member

@bsavitzky bsavitzky 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 - let us know when you're done modifying that import_spec --> try...except statements and I think we can merge

@bsavitzky
Copy link
Member

@sezelt I'm good with these edits but will wait for your approval as well.

Copy link
Member

@sezelt sezelt left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

TODO

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd guess this is a ZeroDivisionError, but I didn't want to guess I can remove the TODO if prefered.

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
Copy link
Member

Choose a reason for hiding this comment

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

TODO

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

TODO

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can remove

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@alex-rakowski
Copy link
Collaborator Author

@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.

@bsavitzky
Copy link
Member

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.

@alex-rakowski alex-rakowski changed the title Clearing up minor issues config_checker update and linting for C408,C419,C405, C409, E721,F401,E722 Nov 21, 2023
@alex-rakowski
Copy link
Collaborator Author

This PR does the following:

Updates config_checker so that it does scrapes the required modules and optional installs from setup.py and adds generic version checking for installed versions of packages. New behavior is if we add a dependency or add a new optional set of dependencies e.g. ~"dev" : ["pytest", "black", "flake8"], it will no automatically pick up on "dev" install optional and test whether all the dependencies are met, and report the state i.e. all met or not. Previously, we had to manually update by adding a dependency or optional dependencies set to a list or dict.

In addition, it lints for the following errors:

Added more minor changes:

@sezelt
Copy link
Member

sezelt commented Mar 8, 2024

I've tried to sync this with dev and merge with #571

@sezelt
Copy link
Member

sezelt commented Mar 8, 2024

It seems to not pass its own tests anymore, though, so something is off with the updated flake8 config...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants