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

get strings from omero.cmd.ERR and omero.CmdError correctly #534

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

will-moore
Copy link
Member

Fixes #481

This fixes 2 similar bugs where we're handling omero.CmdError or omero.cmd.ERR objects and converting into strings.

Both were previously failing with AttributeError: 'ERR' object has no attribute 'message'.

Changes based on using the appropriate attributes as tested at #481 (comment)

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

The usage of str(omero.CmdError) matches the intent of this API introduced in ome/omero-py#151

For the omero.cmd.ERR handling, as an element of comparison the CLI implementation in https://github.com/ome/omero-py/blob/0dc60bd67a5f9281e2063a0a3aeeaa119c14447d/src/omero/cli.py#L1907-L1927 uses rsp.name as the error message and rsp.parameters as the report. Would it make sense to align the behavior in OMERO.web

@will-moore
Copy link
Member Author

I tried str(err) for both cases at #481 (comment) and it gave sub-optimal results.
Not sure exactly what the difference is between omero.CmdError and omero.cmd.ERR!

@will-moore will-moore force-pushed the fix_err_object_has_no_attribute_message branch from dfe2091 to d328a0c Compare March 6, 2024 12:26
@will-moore
Copy link
Member Author

@sbesson updated as suggested to use str(omero.CmdError) and str(omero.cmd.ERR.name)

@sbesson sbesson self-requested a review March 6, 2024 19:39
@will-moore
Copy link
Member Author

cc @knabar This is not an urgent PR, but it is a trivial fix so could maybe get reviewed and included in the next release?

@sbesson
Copy link
Member

sbesson commented Mar 12, 2024

@will-moore I have not had a chance to get back to those. For both updates, do you have scenarios that would allow to reproduce the original errors and asses the impact of the changes functionally?

@will-moore
Copy link
Member Author

@sbesson No, unfortunately I don't have scenarios to reproduce those errors. We only have stack traces from QA, but don't know what caused them (difficult to understand from the stack traces themselves).
I could try guessing a few, but not sure where to start

@knabar
Copy link
Member

knabar commented Mar 13, 2024

@will-moore I'm curious if these missing attributes existed in the past, and if so, when they were removed or renamed. Discussed with @sbesson a bit and he pointed out this could be related to Python 2/3 upgrades as well.

Overall I feel we need a bit more investigation, so I'd like to hold off on this one until 5.26.0, which could come fairly soon anyway.

@will-moore
Copy link
Member Author

@knabar I don't think they've ever existed. It was just assumed that an exception would have a message attribute.

@knabar knabar added this to the 5.26.0 milestone May 7, 2024
@knabar knabar modified the milestones: 5.26.0, 5.27.0 May 21, 2024
@knabar knabar removed this from the 5.27.0 milestone Jul 18, 2024
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.

AttributeError: 'ERR' object has no attribute 'message'
3 participants