Skip to content

fix --rules-dir in mathml2text CLI#543

Open
moritz-gross wants to merge 1 commit intodaisy:mainfrom
moritz-gross:fix-rulesdir-in-mathml2text
Open

fix --rules-dir in mathml2text CLI#543
moritz-gross wants to merge 1 commit intodaisy:mainfrom
moritz-gross:fix-rulesdir-in-mathml2text

Conversation

@moritz-gross
Copy link
Copy Markdown
Collaborator

While testing out the mathml2text CLI, I noticed that --rules-dir seems to not actually be used.
For example, the following shouldn't work right? cargo run --bin mathml2text -- --rules-dir /some/other/rules

Regarding the integration tests, I don't really know what I'm doing there.

* Added `resolve_rules_dir` function to handle `--rules-dir` resolution.
* Updated logging to display the resolved rules directory.
* Added tests ensuring correct behavior for explicit `--rules-dir` usage, default rules-dir fallback, and invalid rules-dir handling.
@masonium
Copy link
Copy Markdown
Contributor

@NSoiffer asked me to take a look. The actual rules_dir fix looks good. I'll leave a couple of other comments inline.

/// Verifies that the positional input-file argument still works on its own.
/// This keeps the default CLI file-input path covered while testing the rules-dir changes separately.
#[test]
fn still_accepts_input_file() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this test is really necessary. In practice I don't think you're really testing anything besides the argument parsing library itself.

/// Verifies that `mathml2text` uses an explicitly provided `--rules-dir`.
/// This protects the CLI override path instead of silently falling back to the default rules location.
#[test]
fn accepts_explicit_rules_dir() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would put most of this boiler plate code in a fixture structure, so you can reuse more of the logic.

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.

2 participants