Bug 529526 - Remove org.eclipse.sirius.ext.base.Option in favor of Java 8's java.util.Optional
Summary: Remove org.eclipse.sirius.ext.base.Option in favor of Java 8's java.util.Opti...
Status: ASSIGNED
Alias: None
Product: Sirius
Classification: Modeling
Component: Core (show other bugs)
Version: 5.1.0   Edit
Hardware: All All
: P5 enhancement (vote)
Target Milestone: Next   Edit
Assignee: Pierre-Charles David CLA
QA Contact:
URL:
Whiteboard:
Keywords: triaged
Depends on:
Blocks:
 
Reported: 2018-01-08 08:32 EST by Pierre-Charles David CLA
Modified: 2022-01-20 10:03 EST (History)
1 user (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre-Charles David CLA 2018-01-08 08:32:45 EST 
The title says it all: we have a custom "Option" type for historical reasons, but it would be better to drop it and use the standard java.util.Optional instead.

There are *a lot* of references to the custom type (a quick search shows 2198 references in my workspace). It would be nice if the replacement could be automated, but I'm not sure it can.

A possible plan:
1. @Deprecate the current type (and the related Options class).
2. Start replacing the API usages first, leaving the purely internal usages for now.
3. If it's still too much, prioritize core plug-ins, then dialect-specific ones, then UI.
Comment 1 Pierre-Charles David CLA 2018-01-08 08:37:32 EST 
(In reply to Pierre-Charles David from comment #0)
> There are *a lot* of references to the custom type (a quick search shows
> 2198 references in my workspace).

Removing all hits from *.internal.* packages still leaves 1202 references (including tests).

Brutally removing the Option & Options types produces 4613 compilation errors (in my workspace, which contains a little more than just the Sirius sources).
Comment 2 Pierre-Charles David CLA 2018-01-20 04:58:00 EST 
Actually, it seems the following script may do the trick:

#!/bin/sh

find . -type f -name "*.java" -not \( -name "Option.java" -o -name "Options.java" \) -print | while read f; do
    sed -i -r \
        -e 's/org.eclipse.sirius.ext.base.Option</java.util.Optional</g' \
        -e 's/\bOption\.some\(/java.util.Optional.isPresent(/g' \
        -e 's/\bOption\.get\(/java.util.Optional.isPresent(/g' \
        -e 's/\bOptions\.newNone\(/java.util.Optional.empty(/g' \
        -e 's/\bOptions\.newSome\(/java.util.Optional.of(/g' \
        -e 's/\bOptions\.fromNullable\(/java.util.Optional.ofNullable(/g' \
        -e 's/\bOption</java.util.Optional</g' \
        -e 's/import org\.eclipse\.sirius\.ext\.base\.Option;//g' \
        -e 's/import org\.eclipse\.sirius\.ext\.base\.Options;//g' \
        -e 's/\bOption::some/java.util.Optional::isPresent/g' \
        -e 's/\bOption::get/java.util.Optional::get/g' \
        -e 's/\.some\(\)/.isPresent()/g' \
        -e 's/\bOptions.<([A-Za-z<\?>]+)> *newNone\(\)/java.util.Optional.empty()/g' \
        -e 's/\bOptions.<([A-Za-z<\?>]+)> *fromNullable\(/java.util.Optional.ofNullable(/g' \
        "$f"
done

The result builds without any error (I have not run the tests), even after removing the old Option & Options classes. A separate "Organize imports" and API change documentation will be needed to finish the job but that's trivial.
Comment 3 Eclipse Genie CLA 2018-01-20 05:13:44 EST 
New Gerrit change created: https://git.eclipse.org/r/115744
Comment 4 Eclipse Genie CLA 2018-01-20 05:13:47 EST 
New Gerrit change created: https://git.eclipse.org/r/115743
Comment 5 Pierre-Charles David CLA 2018-01-22 09:50:47 EST 
Actually, I was a little too optimistic. There are (at least) two remaining issues with the current patch:
* one is purely cosmetic: the "Organize imports" action leaves the fully qualified java.util.Optional references in the code instead of replacing them with a short name + additional import as I expected;
* the other is more problematic: the old Options.newSome() API accepted null as a possible value, producing the equivalent of Options.newNone() in this case. It is actually equivalent to Options.ofNullable(). java.util.Optional.of(null) actually throws an NPE immediately, causing lots of test failures after the transformation. We should probably replace Options.newSome() with Optional.ofNullable(), which is more correct, even though it will highlight the fact that in our current usage of Options.newSome() we very often avoided actually asking ourselves if we'll really return an actual value. But that can be improved over time after the transformation (or not).
Comment 6 Eclipse Genie CLA 2019-01-29 08:54:39 EST 
New Gerrit change created: https://git.eclipse.org/r/135936
Comment 7 Eclipse Genie CLA 2019-01-29 08:54:40 EST 
New Gerrit change created: https://git.eclipse.org/r/135935
Comment 8 Eclipse Genie CLA 2019-01-29 08:54:42 EST 
New Gerrit change created: https://git.eclipse.org/r/135938
Comment 9 Eclipse Genie CLA 2019-01-29 08:54:43 EST 
New Gerrit change created: https://git.eclipse.org/r/135937
Comment 14 Eclipse Genie CLA 2019-02-07 09:55:27 EST 
New Gerrit change created: https://git.eclipse.org/r/136444
Comment 16 Pierre-Charles David CLA 2019-02-11 08:25:18 EST 
Part of this was merged in 6.2.0, the next step will wait for the next version.
Comment 17 Eclipse Genie CLA 2022-01-03 09:36:30 EST 
New Gerrit change created: https://git.eclipse.org/r/c/sirius/org.eclipse.sirius/+/189233
Comment 19 Pierre-Charles David CLA 2022-01-20 10:03:47 EST 
We've marked the type as deprecated for removal, and may remove some of its uses, but it will not be gone for 7.0.