-
Notifications
You must be signed in to change notification settings - Fork 8
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
[LIC-Base] Simplify IO operations #1071
Conversation
e896ba3
to
bb64297
Compare
Hi @HannesWell ! Thank you for PR. |
Understand, then I was misguided by some of the other Plug-ins that already require Java-11. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dear @HannesWell
We appreciate your effort very much.
First of all, you have found a serious bug, which is cool. Thank you!
Regarding main idea of the request: io operations are presented in Passage LIC in old-style facion for a reason: we cannot afford switchig to Java11 for Passage Core for observable future.
While products forming bundles (passage.loc
, passage.fls
) can indeed make use of Java11, all passage.lic
s must stick with Java8. We have integrations with Java8-only products, and thus must respect backward compatibility. Above this, becides Eclipse ecosystem, we build Passage LIC separately to be used in not-eclipse applications - here, too, keeping plain Java8 compatibility matters.
This means, all 8->11 porting work you've done will not pass. Sorry you wasted your resources for it.
Thank you again for spending so many time with our codebase :)
bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/FileCollection.java
Show resolved
Hide resolved
bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/MD5Hashes.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/Settings.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/Settings.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #1071 +/- ##
============================================
- Coverage 33.11% 33.07% -0.04%
Complexity 346 346
============================================
Files 1156 1156
Lines 25623 25612 -11
Branches 1588 1587 -1
============================================
- Hits 8485 8472 -13
- Misses 16634 16636 +2
Partials 504 504
Continue to review full report at Codecov.
|
Dear @HannesWell Please have a look at code review concerns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your detailed review and elaboration. I now understand the constraints of Project passage a bit better.
I implemented your remarks accordingly. :)
The changes that require Java-11 didn't took me long, so it's not a problem that I had to revert them.
bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/FileCollection.java
Show resolved
Hide resolved
bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/MD5Hashes.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/Settings.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/Settings.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HannesWell
IO usage looks ok, there are just some code quality issues
bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/Settings.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/Settings.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/Settings.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/FileCollection.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/Settings.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HannesWell minor notes left.
Make also sure you've updated copyright
headers of changed files according to Eclipse Project Handbook:
- place 2022 to last update position and
- mention yourself in contributors list.
bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/Settings.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/FileCollection.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/FileCollection.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make also sure you've updated
copyright
headers of changed files according to Eclipse Project Handbook:
Sorry, I forgot that, thanks for the reminder. The years are updated in each file and I added myself as contributor in those files where I did more than a single line change.
bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/FileCollection.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did great adopting the changes for the project code quality standards!
One final touch and we are done :)
bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/FileCollection.java
Outdated
Show resolved
Hide resolved
Thanks. I applied your final remark. |
bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/FileCollection.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Hannes Wellmann <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you )
You are welcome and thank you for your reviews. :) |
This PR simplifies the IO operations in the package
org.eclipse.passage.lic.base.io
by better leveraging Java's NIO Path API.Some of the changes require Java-11. But since some other Plug-ins already use Java-11 as their lower bound (just like Eclipse-Platform) I assume this is OK.