Skip to content

Commit 2d867c2

Browse files
bundoleeclaude
andauthored
refactor(core): move CLIOptions to core api.cli package (#475)
* refactor(core): move CLIOptions to core api.cli package Objective: Downstream tools (opendataloader-pdfua) need CLI option parsing logic to populate Config from a CommandLine. They had to depend on opendataloader-pdf-cli, which is not published to Maven Central — forcing a local `mvn install` step before any build. Approach: Move CLIOptions to core under org.opendataloader.pdf.api.cli so downstream tools depend only on core (which is already on Central). The cli module keeps CLIMain as the executable entry point and adds an import to the new package. Stable members for downstream use are documented on the class: defineOptions, addAllTo, applyAllTo, FOLDER_OPTION. Evidence: - mvn test in core: 634/634 pass (incl. moved CLIOptionsTest, CLIOptionsContentSafetyTest) - mvn test in cli: 5/5 pass - npm test (vitest, node wrapper): 32/32 pass - pytest (python wrapper, cli_options + convert_integration): 13/13 pass - `--export-options` byte-level diff vs existing options.json: identical - shaded jar manifest: Main-Class still org.opendataloader.pdf.cli.CLIMain Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * review: tighten CLIOptions stability contract + cover addAllTo edge cases Objective: Code review on #475 flagged that the class-level Javadoc declared four "stable" members but did nothing to discourage downstream from depending on the dozens of other public statics (option-name constants, createConfigFromCommandLine, exportOptionsAsJson). Also asked for direct test coverage of the addAllTo external-Options pattern that pdfua relies on. Approach: - Rewrite the class Javadoc to explicitly mark all non-listed public members as internal — public visibility for cross-package access only, may change in any release. Add a usage example so the intended pattern (custom Options + addAllTo + applyAllTo) is visible at the top of the file. - Add two tests covering the contract pdfua actually depends on: addAllTo preserves pre-existing downstream options, and addAllTo is idempotent (commons-cli's silent-replace-by-long-name behavior is pinned so a future commons-cli upgrade that changes it surfaces here). Evidence: - mvn test CLIOptionsTest: 61/61 pass (incl. 2 new) - mvn install: BUILD SUCCESS - mvn javadoc:jar -P release: BUILD SUCCESS (doclint passes new Javadoc) - Probe of commons-cli 1.11 confirms Options.addOption silently replaces by long-name (count stays 1, last description wins) — test pins this. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent c68c696 commit 2d867c2

5 files changed

Lines changed: 64 additions & 3 deletions

File tree

java/opendataloader-pdf-cli/src/main/java/org/opendataloader/pdf/cli/CLIMain.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import org.apache.commons.cli.*;
1919
import org.opendataloader.pdf.api.Config;
2020
import org.opendataloader.pdf.api.OpenDataLoaderPDF;
21+
import org.opendataloader.pdf.api.cli.CLIOptions;
2122
import org.opendataloader.pdf.containers.StaticLayoutContainers;
2223

2324
import java.io.File;

java/opendataloader-pdf-core/pom.xml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,10 @@
7777
<artifactId>jackson-databind</artifactId>
7878
<version>${jackson.databind.version}</version>
7979
</dependency>
80+
<dependency>
81+
<groupId>commons-cli</groupId>
82+
<artifactId>commons-cli</artifactId>
83+
</dependency>
8084
<dependency>
8185
<groupId>com.squareup.okhttp3</groupId>
8286
<artifactId>okhttp</artifactId>

java/opendataloader-pdf-cli/src/main/java/org/opendataloader/pdf/cli/CLIOptions.java renamed to java/opendataloader-pdf-core/src/main/java/org/opendataloader/pdf/api/cli/CLIOptions.java

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
* See the License for the specific language governing permissions and
1414
* limitations under the License.
1515
*/
16-
package org.opendataloader.pdf.cli;
16+
package org.opendataloader.pdf.api.cli;
1717

1818
import org.apache.commons.cli.CommandLine;
1919
import org.apache.commons.cli.Option;
@@ -30,6 +30,37 @@
3030
import java.util.Set;
3131
import java.util.stream.Collectors;
3232

33+
/**
34+
* Adapter that maps Apache Commons CLI options to {@link Config} / {@link HybridConfig}.
35+
*
36+
* <p><b>Stable API for downstream tools</b> (e.g. opendataloader-pdfua) — these four
37+
* members are the supported integration surface and will not break compatibly:
38+
* <ul>
39+
* <li>{@link #defineOptions()} — get a fully populated {@code Options} instance</li>
40+
* <li>{@link #addAllTo(Options)} — add core options into an externally-built {@code Options}</li>
41+
* <li>{@link #applyAllTo(Config, CommandLine)} — populate a {@code Config} from a parsed line</li>
42+
* <li>{@link #FOLDER_OPTION} — short option name for {@code --output-dir}</li>
43+
* </ul>
44+
*
45+
* <p><b>Everything else is internal.</b> The numerous other public {@code static} members
46+
* (option-name constants, helpers like {@code createConfigFromCommandLine},
47+
* {@code exportOptionsAsJson}) exist for the CLI module ({@code CLIMain}) and the
48+
* options-export tooling that drives Node/Python binding generation. Their visibility
49+
* is {@code public} only for cross-package access within this codebase; they are
50+
* <i>not</i> part of the supported API and may be renamed, moved, or removed in any
51+
* release. Downstream consumers depending on them do so at their own risk.
52+
*
53+
* <p>Pdfua's usage pattern (build your own {@code Options}, add core's, parse, then
54+
* populate {@code Config}):
55+
* <pre>{@code
56+
* Options options = new Options();
57+
* options.addOption(...); // your tool-specific options
58+
* CLIOptions.addAllTo(options); // add core's options
59+
* CommandLine cmd = parser.parse(options, args);
60+
* Config core = new Config();
61+
* CLIOptions.applyAllTo(core, cmd);
62+
* }</pre>
63+
*/
3364
public class CLIOptions {
3465

3566
// ===== Output Directory =====

java/opendataloader-pdf-cli/src/test/java/org/opendataloader/pdf/cli/CLIOptionsContentSafetyTest.java renamed to java/opendataloader-pdf-core/src/test/java/org/opendataloader/pdf/api/cli/CLIOptionsContentSafetyTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
* See the License for the specific language governing permissions and
1414
* limitations under the License.
1515
*/
16-
package org.opendataloader.pdf.cli;
16+
package org.opendataloader.pdf.api.cli;
1717

1818
import org.apache.commons.cli.CommandLine;
1919
import org.apache.commons.cli.CommandLineParser;

java/opendataloader-pdf-cli/src/test/java/org/opendataloader/pdf/cli/CLIOptionsTest.java renamed to java/opendataloader-pdf-core/src/test/java/org/opendataloader/pdf/api/cli/CLIOptionsTest.java

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
* See the License for the specific language governing permissions and
1414
* limitations under the License.
1515
*/
16-
package org.opendataloader.pdf.cli;
16+
package org.opendataloader.pdf.api.cli;
1717

1818
import org.apache.commons.cli.CommandLine;
1919
import org.apache.commons.cli.CommandLineParser;
@@ -639,4 +639,29 @@ void testApplyAllTo_appliesCoreOptions() throws ParseException {
639639
// downstream-only is not consumed by applyAllTo — that is the caller's job.
640640
assertEquals("value", cmd.getOptionValue("downstream-only"));
641641
}
642+
643+
@Test
644+
void testAddAllTo_preservesPreexistingDownstreamOptions() {
645+
Options ext = new Options();
646+
ext.addOption(null, "ua-foo", true, "Pdfua-specific option");
647+
ext.addOption(null, "ua-bar", false, "Pdfua-specific flag");
648+
CLIOptions.addAllTo(ext);
649+
650+
assertTrue(ext.hasOption("ua-foo"));
651+
assertTrue(ext.hasOption("ua-bar"));
652+
assertTrue(ext.hasOption("hybrid"));
653+
assertTrue(ext.hasOption("threads"));
654+
}
655+
656+
@Test
657+
void testAddAllTo_calledTwice_isIdempotent() {
658+
Options ext = new Options();
659+
CLIOptions.addAllTo(ext);
660+
int afterFirst = ext.getOptions().size();
661+
// commons-cli's Options.addOption silently replaces by long-name, so calling
662+
// addAllTo twice does not throw and does not duplicate. Pin this so a future
663+
// commons-cli upgrade that changes the behavior surfaces here.
664+
CLIOptions.addAllTo(ext);
665+
assertEquals(afterFirst, ext.getOptions().size());
666+
}
642667
}

0 commit comments

Comments
 (0)