executablebooks / rst-to-myst

Convert ReStructuredText to MyST Markdown
https://rst-to-myst.readthedocs.io
MIT License
59 stars 10 forks source link

🐛 FIX: Glob path works as intended (#35) #39

Open Joinyy opened 1 year ago

Joinyy commented 1 year ago

Replace click.Path with the click-path GlobPath to have the CLI work as intended in the readme.

welcome[bot] commented 1 year ago

Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.
Welcome to the EBP community! :tada:

codecov[bot] commented 1 year ago

Codecov Report

Base: 84.40% // Head: 84.40% // No change to project coverage :thumbsup:

Coverage data is based on head (e6d2fa6) compared to base (6e51d18). Patch has no changes to coverable lines.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #39 +/- ## ======================================= Coverage 84.40% 84.40% ======================================= Files 9 9 Lines 1629 1629 ======================================= Hits 1375 1375 Misses 254 254 ``` | Flag | Coverage Δ | | |---|---|---| | pytests | `84.40% <ø> (ø)` | | 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. 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.

chrisjsewell commented 1 year ago

Heya, thanks for the PR!

to have the CLI work as intended in the readme.

Erm when you say it doesn't work, how are you using it?

For unix/osx terminals they already handle the glob expansion, e.g.:

rst-to-myst$ echo docs/**/*.rst
docs/source/api.rst docs/source/cli.rst

So it certainly works for me as intended:

rst-to-myst/$ rst2myst convert docs/**/*.rst
docs/source/api.rst -> docs/source/api.md
CONVERTED (extensions: [])
docs/source/cli.rst -> docs/source/cli.md
CONVERTED (extensions: [])
chrisjsewell commented 1 year ago

Also, looking at the code for the added extension, its very minimal: https://gitlab.com/abraxos/click-path/-/blob/master/click_path/core.py#L10

So if we were to do something like this, I would rather just have the code, and avoid the extra dependency 😅

chrisjsewell commented 1 year ago

ah I see its meant to fix for windows, but yeh I wouldadd the code here for the paramtype, and also would be good to have a test in tests/test_cli.py

Joinyy commented 1 year ago

Yes exactly, it does currently not work on windows except if you specify every file separately. Theoretically this should maybe be fixed within the click package, as the click.Path behaves different on Linux and Windows… After the weekend I will remove the additional dependency and just put in the type directly as well as a test case 😊

Joinyy commented 1 year ago

So I have done some tests and even though it is fixable with the above mentionned code there seems to be another issue here. While testing on a different laptop, I could not get the previously mentionned error on windows, there I used python 3.8.5. After installing python 3.10.7 (newest) the error was reproducible again. This is due to python 3.10 installing click in the version 7.1.2 by default (don't know why...). When installing the newest click (8.3.1) specifically it works perfectly fine like it is on the main branch, so there no work would be needed even on windows. I would suggest to bump the click version in the pyproject.toml to 8.1.3, as this will fix the error / enables the click.Path to work on windows like it was intended by click in the first place.

chrisjsewell commented 1 year ago

This is due to python 3.10 installing click in the version 7.1.2 by default (don't know why...)

yes it would be nice to know why this 🤔