search

ome / omero-blitz

Gradle project containing Ice remoting code for OMERO
https://www.openmicroscopy.org/omero
GNU General Public License v2.0
0 stars 15 forks source link

Add deprecation warnings for generated Blitz model objects. #62

Closed mtbc closed 5 years ago

mtbc commented 5 years ago

This PR completes the work of https://github.com/ome/omero-model/pull/33 by adding deprecation warnings to generated model classes for Ice, Java and Python. The Python warnings should be invisible except to developers using the -Wd option or similar. Files from omero-blitz/build/ that may be interesting to compare for changes include:

rgozim commented 5 years ago

Everything still compiles okay and looking over some of the generated classes (python and ice), the deprecated warnings are where they should be.

Not sure if theres a better way to test this?

mtbc commented 5 years ago

I guess the super-keen could adapt https://github.com/ome/omero-model/pull/32 to try deprecating a couple of other kinds of property to see if that warning code also looks correct. Can't think that it's practical to do much more than that though, I agree.

joshmoore commented 5 years ago

Bit surprised that this doesn't print the deprecation:

>>> import omero
>>> import omero.all
>>> omero.model.ImportJobI()
object #0 (::omero::model::ImportJob)

Note also:

>>> omero.model.ImportJobI.warn_of_deprecation()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unbound method warn_of_deprecation() must be called with ImportJobI instance as first argument (got nothing instead)
mtbc commented 5 years ago

Weird, I'll re-test on my side and report back.

mtbc commented 5 years ago 
(omero) mtbc@ls28101:~/src/openmicroscopy$ python -Wd
Python 2.7.13 (default, Sep 26 2018, 18:42:22) 
[GCC 6.3.0 20170516] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import omero
>>> import omero.all
/home/mtbc/src/openmicroscopy/dist/lib/python/omero_model_ImportJobI.py:25: DeprecationWarning: ImportJob is deprecated
  warnings.warn("ImportJob is deprecated", DeprecationWarning)
/home/mtbc/src/openmicroscopy/dist/lib/python/omero_model_PathI.py:25: DeprecationWarning: Path is deprecated
  warnings.warn("Path is deprecated", DeprecationWarning)
>>>
joshmoore commented 5 years ago

Gotcha. Thanks. Would it be possible to warn on instantiation rather than import with or without an extra flag?

mtbc commented 5 years ago

Can warn on instantiation instead. On only the first or on every instantiation of the class? "with or without": okay if it still requires the flag or are you hoping it would warn even without? (If so, also for Java etc.? Though maybe can't assume LoggerFactory is the way.)

joshmoore commented 5 years ago
mtbc commented 5 years ago

On first instantiation: maybe also counts for methods too then? E.g., if PixelsI.getRelatedTo is called then warn but then don't for when setRelatedTo is then called? I can probably figure that out. (I'm not blocking any other combined.vm changes with this PR?)

Java: Currently doing no more than adding @Deprecated annotations so usage won't signal anything. Could look at making more parallel to the Python code except I don't think there's a warnings.warn that I can be confident makes sense.

joshmoore commented 5 years ago

Java:... I don't think there's a warnings.warn that I can be confident makes sense. Sorry, I was having difficulty switching gears. In the Java world, the first "usage" is at compile time (without extra flags), which seems fine.

joshmoore commented 5 years ago 
$ bin/omero shell
In [1]: import omero

In [2]: omero.model.ImportJobI()
/home/omero/workspace/OMERO-training/OMERO.server/lib/python/omero_model_ImportJobI.py:32: DeprecationWarning: ImportJob is deprecated
  warnings.warn(item + " is deprecated", DeprecationWarning)
Out[2]:
object #0 (::omero::model::ImportJob)
{
...
In [3]: omero.model.ImportJobI()
Out[3]:
object #0 (::omero::model::ImportJob)
...
In [4]: omero.model.PixelsI().getRelatedTo()
/home/omero/workspace/OMERO-training/OMERO.server/lib/python/omero_model_PixelsI.py:32: DeprecationWarning: Pixels.relatedTo is deprecated
  warnings.warn(item + " is deprecated", DeprecationWarning)

In [5]: omero.model.PixelsI().getRelatedTo()

In [6]:

LGTM, thanks, @mtbc.

rgozim commented 5 years ago

Looks okay. Not seen any issues building omero with this PR.