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

Update HERD.get_key to replace ValueError with return values #964

Closed
wants to merge 2 commits into from

Conversation

oruebel
Copy link
Contributor

@oruebel oruebel commented Oct 7, 2023

Motivation

HERD.get_key currently raises a number of different errors. I propose to change the errors to more useful return values:

  • If no key is found return None instead of raising a ValueError
  • If multiple matching keys are found, then return a list of Key objects rather than raising a ValueError

Alternatively, we could also always return a list of Key objects such that the list is empty when no Key is found, has 1 element when an exact match is found, and has multiple elements if many keys match.

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Have you checked our Contributing document?
  • Have you ensured the PR clearly describes the problem and the solution?
  • Is your contribution compliant with our coding style? This can be checked running ruff from the source directory.
  • Have you checked to ensure that there aren't other open Pull Requests for the same change?
  • Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

@oruebel
Copy link
Contributor Author

oruebel commented Oct 7, 2023

@mavaylon1 what do you think. What is the most appropriate behavior for HERD.get_key?

@mavaylon1
Copy link
Contributor

@mavaylon1 what do you think. What is the most appropriate behavior for HERD.get_key?

So this kind of comes to what I was thinking for "automatically resolving keys that exist". The user only calls this method when reusing a key for add_ref. If we actually make this resolution automatic in add_ref, then the get_key would most likely be a private method to aid that resolution (with modifications). At the end of it, the behavior of get_key is probably going to change, but your changes are also fine for the time being. Thoughts?

Another change we should do:
I think one idea is to check the existence of keys and entities before anything else in add_ref. This way we don't partially populate the HERD tables because that does happen if we catch the error down the road as we do now.

@oruebel
Copy link
Contributor Author

oruebel commented Oct 8, 2023

I think one idea is to check the existence of keys and entities before anything else in add_ref. This way we don't partially populate the HERD tables because that does happen if we catch the error down the road as we do now.

Yes, we should do error checks first

then the get_key would most likely be a private method to aid that resolution (with modifications)

I think having a way for users to lookup keys will be useful (same for entities, objects, and files), not because they are needed for add_ref, but as a means for the user to query HERD.

@oruebel
Copy link
Contributor Author

oruebel commented Oct 8, 2023

So this kind of comes to what I was thinking for "automatically resolving keys that exist". T

I think for keys we probably need a flag to indicate whether to reuse keys or create a new key.

@mavaylon1 mavaylon1 closed this Oct 31, 2023
@mavaylon1 mavaylon1 reopened this Oct 31, 2023
@mavaylon1 mavaylon1 mentioned this pull request Oct 31, 2023
13 tasks
@mavaylon1
Copy link
Contributor

I don't think changing to None is more informative. I like the fact the error tells you why. As for the second one, I think that's fine. I will continue this PR.

@mavaylon1 mavaylon1 closed this Nov 21, 2023
@rly rly deleted the prop/herd_get_key branch January 23, 2024 01:52
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.

2 participants