RFR: JDK-8230521: rename --output/-o option and add default value (".")
Alexey Semenyuk
alexey.semenyuk at oracle.com
Fri Sep 13 02:47:10 UTC 2019
http://cr.openjdk.java.net/~almatvee/8230521/webrev.00/src/jdk.jpackage/share/classes/jdk/jpackage/internal/Arguments.java.sdiff.html:
---
126 Path destPath = Paths.get(".").toAbsolutePath();
127 if (destPath.getFileName().toString().equals(".")) {
128 dest = destPath.getParent().toString();
129 } else {
130 dest = destPath.toString();
131 }
---
Are you trying to get rid of trailing "." path component in the above
code fragment? If yes, then this can be simplified as follows:
---
Path destPath = Paths.get(".").normalize().toAbsolutePath();
---
In general I don't like the idea of renaming variables and method names
from `output` to `dest`. This doesn't add much value to the code and is
incomplete and thus confusing (e.g.: in all the tests OUTPUT variable
remains unnamed).
I suggest to change only `--output` to `--dest` tokens in the code.
Slightly unrelated thing. macOS tests
---
test/jdk/tools/jpackage/macosx/base/Base.java
test/jdk/tools/jpackage/macosx/base/FileAssociationsBase.java
test/jdk/tools/jpackage/macosx/base/InstallDirBase.java
test/jdk/tools/jpackage/macosx/base/LicenseBase.java
---
duplicate corresponding shared tests that have been added in the patch
for https://bugs.openjdk.java.net/browse/JDK-8230726. I didn't touch
them as I didn't have MAC to try the replacements. Could you please just
drop these obsolete tests and also run the new tests to make sure they
work as expected. I mean not just run the tests to create package
bundles, but also install these bundles and run the tests in a mode when
they verify packages are properly installed.
- Alexey
On 9/12/2019 9:34 PM, Alexander Matveev wrote:
> Please review the jpackage fix for bug [1] at [2].
>
> This is a fix for the JDK-8200758-branch branch of the open sandbox
> repository (jpackage).
>
> - Renamed "--output" to "--dest" and made it optional with default to
> ".".
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8230521
>
> [2] http://cr.openjdk.java.net/~almatvee/8230629/webrev.00/
>
> Thanks,
> Alexander
More information about the core-libs-dev
mailing list