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

Improve error catching and message for add #290

Merged
merged 24 commits into from
Nov 26, 2024
Merged

Conversation

chStaiger
Copy link
Collaborator

Improve error message as decsribed in #289

@chStaiger chStaiger requested a review from qubixes November 25, 2024 15:57
Copy link
Collaborator

@qubixes qubixes left a comment

Choose a reason for hiding this comment

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

Thanks! I do think the old way had some advantages that we lose in the new implementation. Namely, if PRC / server implements these and there is no more error, the code will immediately work on our side as well. Perhaps the best would be to catch the TypeError, and reraise it with the new text if key/value is None, else reraise the original error.

ibridges/meta.py Outdated Show resolved Hide resolved
ibridges/meta.py Outdated Show resolved Hide resolved
ibridges/meta.py Outdated
@@ -203,6 +203,10 @@ def add(self, key: str, value: str, units: Optional[str] = None):
>>> meta.add("Mass", "10", "kg")

"""
if key is None or key == "":
raise ValueError("Key cannot be None or empty.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think technically, key is None should raise a TypeError and key == "" should raise a ValueError. Perhaps it makes the most sense to raise a TypeError in this case? We could also split it, but that may be too much for this technical detail.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TO be pragmatic, I will turn them into TypeErrors.

@chStaiger
Copy link
Collaborator Author

chStaiger commented Nov 25, 2024

I tried to trigger the except irods.message.Bad_AVU_Field, but it crashes beforehand in e.g. our blacklist check with regular expressions. So we de facto we are not making any use of this error.
Here a list:

  • if the key is None the regex fails
  • if the key is '' we get this error from the PRC: ValueError: Empty value in <iRODSMeta None bla None>
  • The PRC does not catch the error if the value is None: TypeError: object of type 'NoneType' has no len()
  • And if the value is '' we get the same error as above ValueError: Empty value in <iRODSMeta None Bla None>. So from those errors it is not clear whether the key causes the problem or the value

@qubixes
Copy link
Collaborator

qubixes commented Nov 25, 2024

I tried to trigger the except irods.message.Bad_AVU_Field, but it crashes beforehand in e.g. our blacklist check with regular expressions. So we de facto we are not making any use of this error. Here a list:

* if the key is None the regex fails

* if the key is `''` we get this error from the PRC: `ValueError: Empty value in <iRODSMeta None  bla None>`

* The PRC does not catch the error if the value is None: `TypeError: object of type 'NoneType' has no len()`

* And if the value is `''` we get the same error as above `ValueError: Empty value in <iRODSMeta None Bla  None>`. So from those errors it is not clear whether the key causes the problem or the value

I'm quite sure I have seen the error at some point. What about other types? Like key/value being an integer? Does the blacklist check error you get when key == None make sense?

@chStaiger
Copy link
Collaborator Author

chStaiger commented Nov 25, 2024

When I use a list or a tuple, then I hit the irods exception TypeError: Value should have type str or bytes-like, not <class 'list'>.. For integers I get the TypeError: object of type 'int' has no len()

@chStaiger
Copy link
Collaborator Author

The error from the re.match function when provided None is a bit meager:
TypeError: expected string or bytes-like object, got 'NoneType'

The user does not really know what went wrong and which part of the metadata item is the culprit.

@chStaiger
Copy link
Collaborator Author

I think I got it now right. In the "add" function there is an extra TypeError that catches the failure of the comparison between ley and blacklist. I added again the BadAVU exception and only replaced its message when something is wrong with the key to stay consitstent that in iBridges we call attributes keys.

@qubixes Can you please review again.

@chStaiger chStaiger requested a review from qubixes November 26, 2024 09:17
Copy link
Collaborator

@qubixes qubixes left a comment

Choose a reason for hiding this comment

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

Still some questions, but we're almost there!

ibridges/meta.py Outdated Show resolved Hide resolved
ibridges/meta.py Outdated Show resolved Hide resolved
ibridges/meta.py Outdated Show resolved Hide resolved
ibridges/meta.py Outdated Show resolved Hide resolved
ibridges/meta.py Outdated Show resolved Hide resolved
ibridges/meta.py Outdated
try:
self._ibridges_meta.add(*new_item_key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should be in the old place. Now, we don't know whether the new one was already added.

ibridges/meta.py Outdated Show resolved Hide resolved
ibridges/meta.py Show resolved Hide resolved
@@ -139,7 +139,7 @@ def search_data( # pylint: disable=too-many-branches
path = IrodsPath(session, path)

if metadata is None:
metadata = []
metadata = [] # type: ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Which error are we ignoring here? Can't we add an annotation?

@@ -178,7 +178,7 @@ def search_data( # pylint: disable=too-many-branches
if path_pattern is not None:
_path_filter(path_pattern, queries)

for mf in metadata:
for mf in metadata: # type: ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

@chStaiger chStaiger merged commit a6e8c63 into develop Nov 26, 2024
12 checks passed
@chStaiger chStaiger deleted the improve-errmsg branch November 26, 2024 14:43
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