goxberry commented 1 year ago

Attempting to run rst2myst using a YAML configuration file similar to that in the project's documentation

language: en
sphinx: true
extensions:
- sphinx_design
default_domain: py
consecutive_numbering: true
colon_fences: true
dollar_math: true
conversions:
  sphinx_design.dropdown.DropdownDirective: parse_all

on a reStructuredText file with contents

Dropdowns
=========

Example dropdown

.. dropdown:: Dropdown title
    :open:
    :color: info

    Dropdown content

will yield

(.venv) goxberry@penguin:~/repos/github.com/executable-books/rst-to-myst$ rst2myst convert --config test_config.yaml test_sphinx_design_dropdown.rst 
Usage: rst2myst convert [OPTIONS] [PATHS]...
Try 'rst2myst convert -h' for help.

Error: Path does not exist: {'sphinx_design.dropdown.DropdownDirective': 'parse_all'}

in the output.

Similarly, omitting the conversions: key (and its children) from the YAML config file above, but otherwise repeating the rst2myst same command will yield

(.venv) goxberry@penguin:~/repos/github.com/executable-books/rst-to-myst$ rst2myst convert --config test_config.yaml test_sphinx_design_dropdown.rst 
test_sphinx_design_dropdown.rst -> test_sphinx_design_dropdown.md
FAILED:
("Could not import extension ['sphinx_design']", ModuleNotFoundError('No module named "[\'sphinx_design\']"'))

FINISHED ALL! (extensions: [])

in the output.

This pull request adds a test for these failures that uses the --dry-run option, and adds changes to resolve the failures.

The pull request probably needs some work, but the basic idea is that Click will convert the correctly parsed YAML input for the --extensions and --conversions arguments into strings passed to the value parameters of the split_extensions and read_conversions methods in rst_to_myst.cli. Using the example above, the value of the value parameter passed to rst_to_myst.cli.split_extensions is "['sphinx_design']", instead of a list, and the value of the value parameter passed to rst_to_myst.cli.read_conversions is "{'sphinx_design.dropdown.DropdownDirective': 'parse_all'}" instead of a dictionary (or other mapping type).

The fixes proposed in the pull request are to:

In rst_to_myst.cli.split_extensions, test if the value parameter contains [ or ] characters. If it does, attempt to use Python's standard library ast module to convert the string to a list.
In rst_to_myst.cli.read_conversions, test if the value parameter contains { or } characters. If it does, attempt to use Python's standard library ast module to convert the string to a dictionary.

The latter fix is probably not as robust as it could be, in that, e.g., "{'sphinx_design.dropdown.DropdownDirective': 'parse_all'}" is probably a legal POSIX filename -- if I remember correctly, the main restriction for POSIX filenames is that identifiers cannot contain null characters. For now, the fix assumes a user does not intend to pass a path to a conversions YAML configuration file containing braces because it seemed like a good stopping point to solicit maintainer feedback.

codecov[bot] commented 1 year ago

Codecov Report

Base: 84.48% // Head: 84.55% // Increases project coverage by +0.07% :tada:

Coverage data is based on head (183d815) compared to base (53f1adb). Patch coverage: 71.42% of modified lines in pull request are covered.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #52 +/- ## ========================================== + Coverage 84.48% 84.55% +0.07% ========================================== Files 10 10 Lines 1637 1651 +14 ========================================== + Hits 1383 1396 +13 - Misses 254 255 +1 ``` | Flag | Coverage Δ | | |---|---|---| | pytests | `84.55% <71.42%> (+0.07%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/executablebooks/rst-to-myst/pull/52?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks) | Coverage Δ | | |---|---|---| | [rst\_to\_myst/cli.py](https://codecov.io/gh/executablebooks/rst-to-myst/pull/52?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks#diff-cnN0X3RvX215c3QvY2xpLnB5) | `83.98% <71.42%> (+0.12%)` | :arrow_up: | | [rst\_to\_myst/parser.py](https://codecov.io/gh/executablebooks/rst-to-myst/pull/52?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks#diff-cnN0X3RvX215c3QvcGFyc2VyLnB5) | `95.74% <0.00%> (+1.06%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

goxberry commented 1 year ago

I decided it might be better to ensure the example YAML configuration file in the documentation is tested, so I replaced sphinx_panels/sphinx-panels in that example with sphinx_design/sphinx-design, made it a YAML file, and used a literalinclude directive to include it in the documentation.

goxberry commented 1 year ago

@chrisjsewell As far as I can tell, the only CI failure is the codecov patch coverage test. Otherwise, it should be ready for review.

chrisjsewell commented 1 year ago

I think this can be closed then thanks

executablebooks / rst-to-myst

fix: make the example configuration file function correctly #52

Codecov Report