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

Deprecate LegacyND2Reader #4030

Merged
merged 2 commits into from
Jul 5, 2023
Merged

Conversation

sbesson
Copy link
Member

@sbesson sbesson commented Jun 29, 2023

This reader depends on an outdated DLL which has not been built in years, is untested and fully superseded by the new NativeND2Reader

In addition to the Java reader, the following components are scheduled for removal in Bio-Formats 7:

  • lib/LegacyND2Reader.dll
  • components/formats-gpl/src/loci/formats/in/loci_formats_in_LegacyND2Reader.cpp
  • components/formats-gpl/src/loci/formats/in/loci_formats_in_LegacyND2Reader.h

Possibly biggest question is whether we want to keep the delegation logic implemented in ND2Reader which allows to select between the LegacyND2Reader and NativeND2Reader. Listing a series of options briefly discussed with @melissalinkert are:

  1. redirect
    legacyReader = new LegacyND2Reader();
    to NativeND2Reader.
  2. keep ND2Reader but have it simply extend NativeND2Reader
  3. remove ND2Reader and update readers.txt to use NativeND2Reader
  4. remove the delegate reader and rename NativeND2Reader as ND2Reader

Given this is a backwards-incompatible change, my personal preference would go towards options 3 or 4 but definitely open to feedback. In case we go for the latter options, we might want to add other deprecation flags to some of these classes

This reader depends on an outdated DLL which has not been built in
years, is untested and fully superseded by the new NativeND2Reader
Copy link
Member

@melissalinkert melissalinkert left a comment

Choose a reason for hiding this comment

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

Option 4, 3, 2, 1 would be my order of preference for handling ND2Reader.

@dgault
Copy link
Member

dgault commented Jun 30, 2023

We will also want to add warnings to the format pages for each of these similar to Woolz ome/bio-formats-documentation#308

@sbesson
Copy link
Member Author

sbesson commented Jun 30, 2023

Adding to Monday's agenda, we should decide on our removal strategy - see #4030 (review). In particular if we decide to go with option 3 or and 4, I could add a commit to annotate the classes that will be renamed with @deprecated to notify developers.

@joshmoore
Copy link
Member

Brief discussion of an additional option:

  1. move all logic to ND2Reader and have NativeND2Reader subclass it

It's not a strong opinion, but as came out in the chat, I would tend to opt for the least breaking change (but perhaps only since if I started letting myself adjust namings, I probably wouldn't stop... 😉) So a slight preference for 1 (with a warning if anyone chooses legacyReader) but a slightly stronger partial-preference for 5>4 with a deprecation of the updated NativeND2Reader subclass.

@sbesson
Copy link
Member Author

sbesson commented Jul 4, 2023

Summarizing the weekly discussion, the preferred option seems to be migrate the NativeND2Reader logic into ND2Reader. For backwards compatibility, NativeND2Reader might be retained and simplified as a simple extension for as mentioned in #4030 (comment).

In all cases, this means NativeND2Reader is deprecated and consumers should be encouraged to depend directly on ND2Reader. Pushed 74f337c to try and capture this message although open to better ways to express this.

@sbesson sbesson requested a review from joshmoore July 4, 2023 10:44
@dgault dgault merged commit a9fe437 into ome:develop Jul 5, 2023
17 checks passed
@sbesson sbesson deleted the legacynd2reader_deprecation branch July 6, 2023 07:55
This was referenced Jul 14, 2023
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.

4 participants