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

Add more descriptive error if sp and id are not provided #915

Merged
merged 4 commits into from
Apr 25, 2023

Conversation

cbkerr
Copy link
Member

@cbkerr cbkerr commented Apr 4, 2023

Description

Adds a better error messgae for calling project.open_job().

Motivation and Context

Before, it would tell you something that implied you provided both statepoint and id: "Either statepoint or id must be provided, but not both."

I was working on something for dashboard and this more descriptive message would have saved me a little time.

Checklist:

@cbkerr cbkerr requested review from a team as code owners April 4, 2023 19:03
@cbkerr cbkerr requested review from mikemhenry and Tobias-Dwyer and removed request for a team April 4, 2023 19:03
@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Merging #915 (ac75665) into main (522944a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #915   +/-   ##
=======================================
  Coverage   86.64%   86.65%           
=======================================
  Files          41       41           
  Lines        4575     4577    +2     
  Branches      996      997    +1     
=======================================
+ Hits         3964     3966    +2     
  Misses        431      431           
  Partials      180      180           
Impacted Files Coverage Δ
signac/project.py 86.51% <100.00%> (+0.04%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Collaborator

@mikemhenry mikemhenry left a comment

Choose a reason for hiding this comment

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

I am in favor of improving this error message, but I am not sure if id_none = id is None is something I understand that well. I would rather see the None checks directly then aliasing them like that personally. I could be easily persuaded (other examples of us doing this in the codebase? Is this the pythonic way?).

@cbkerr
Copy link
Member Author

cbkerr commented Apr 4, 2023

I am in favor of improving this error message, but I am not sure if id_none = id is None is something I understand that well. I would rather see the None checks directly then aliasing them like that personally. I could be easily persuaded (other examples of us doing this in the codebase? Is this the pythonic way?).

I defined the variables id_none and statepoint_none to only check the is once. But I didn't benchmark if that actually improves performance

@bdice
Copy link
Member

bdice commented Apr 23, 2023

I defined the variables id_none and statepoint_none to only check the is once. But I didn't benchmark if that actually improves performance

It is very unlikely that this will improve performance, in my estimation. x is None is a very cheap check. Let's aim for readability and check is None explicitly. I'll push a commit for that.

@bdice bdice requested a review from mikemhenry April 23, 2023 21:22
@bdice
Copy link
Member

bdice commented Apr 23, 2023

@cbkerr @mikemhenry I'm happy with the changes I just pushed. If you're happy with it, feel free to approve/merge.

@bdice bdice merged commit d87a6fd into main Apr 25, 2023
@bdice bdice deleted the fix/better-error-no-sp-id branch April 25, 2023 01:30
@csadorf csadorf added this to the v2.1.0 milestone Jul 12, 2023
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.

4 participants