ome / omero-web

Django-based OMERO.web client
https://www.openmicroscopy.org/omero
17 stars 31 forks source link

Refactors #555 to allow for backward compatibility #577

Closed chris-allan closed 2 months ago

chris-allan commented 3 months ago

There were certain downstream omero-web plugins which were using _marshal_image(). This refactors the changes of #555 such that the prototype of _marshal_image() is maintained and a new _marshal_image_map() is added to give us options going forward for scalability.

Alternative to #575.

/cc @Tom-TBT

chris-allan commented 3 months ago

I've opened this as a draft so we don't end up with build conflicts with #575 and can use this for discussion.

sbesson commented 3 months ago

With OMERO.web 5.27.0 and OMERO.autotag 4.0.1, I can reproduce the original issue as toggling the Auto-tag from the centre panel triggers a 500 error.

I deployed a local build of this PR with the additional change proposed in https://github.com/ome/omero-web/pull/577#discussion_r1734342465, I can confirm that the auto-tag server issue is fixed. Setting the archived flag to true via the CLI also produces the expected behavior as per #555 i.e. an archived icon and Archived: True in the right-hand panel.

I would be in favor of closing #575 and moving this out of draft for inclusion in the nightly CI builds in preparation of a round of functional testing.

knabar commented 3 months ago

Do we also want to address the issue of the "underscore" method that indicates it is private, since we know it is not used as such?

For example, remove the leading underscore from the methods, but keep the underscore reference for compatibility:

def marshal_image(...):
    ...

_marshal_image = marshal_image

def marshal_image_map(...):
    ...

This is admittedly a bit redundant, but might deter future signature changes.

chris-allan commented 3 months ago

Should the legacy _marshal_image API be tested and/or deprecated?

I think at least tested. Once the current integration tests are passing I'll add some more unit tests to this project on this PR.

chris-allan commented 3 months ago

Do we also want to address the issue of the "underscore" method that indicates it is private, since we know it is not used as such?

For example, remove the leading underscore from the methods, but keep the underscore reference for compatibility:

def marshal_image(...):
    ...

_marshal_image = marshal_image

def marshal_image_map(...):
    ...

This is admittedly a bit redundant, but might deter future signature changes.

I don't think so. Or at least not here. What was clear from #575, especially ome/omero-mapr#84, is that almost every leading underscore marshal method from the tree package is used in that plugin and has been for nearly a decade. The whole module needs review and an assessment of what we we want to do with it as there are clearly critical plugins that are tightly coupled to nearly its entire API.

chris-allan commented 2 months ago

Unit tests have been added to cover both _marshal_image() and _marshal_image_map().