-
Notifications
You must be signed in to change notification settings - Fork 4
Makes .mdformat.toml take precedence
#20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Also, forgot to mentionned, but this PR fixes an issue with mdformat 1.0.0. The patched Traceback (most recent call last):
File "/home/runner/.local/bin/mdformat", line 10, in <module>
sys.exit(run())
^^^^^
File "/home/runner/.local/share/uv/tools/mdformat/lib/python3.12/site-packages/mdformat/__main__.py", line 8, in run
exit_code = mdformat._cli.run(sys.argv[1:])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/runner/.local/share/uv/tools/mdformat/lib/python3.12/site-packages/mdformat/_cli.py", line 56, in run
opts = {**DEFAULT_OPTS, **toml_opts, **cli_core_opts}
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: 'NoneType' object is not a mappingSee my CI for more details: https://github.com/kdeldycke/workflows/actions/runs/18754525944/job/53502912176#step:7:20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some comments. I believe that the current approach may lead to silent conflicts that may be confusing to the user (user edits options in pyproject.toml but observes no behavior changes because a .mdformat.toml exists that they did not spot at first)
905f845 to
5e02d23
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #20 +/- ##
==========================================
- Coverage 92.50% 91.48% -1.02%
==========================================
Files 2 2
Lines 40 47 +7
==========================================
+ Hits 37 43 +6
- Misses 3 4 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5c80fb3 to
0a85fda
Compare
|
I addressed all the issues but there are conflicting reports of the pre-commits hooks because of |
0a85fda to
6da7376
Compare
OK I decided to aligns the configuration files to prevent any mismatch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kdeldycke Thank you very much!
There is one last thing: the tests now are a bit redundant, because test_search_config_directory_inside_project is almost the same as test_patched_read_toml_opts_with_pyproject, and same with the without equivalents.
On the other hand, because .mformat.toml exists, now there is no test in which the options are actually loaded from the pyproject.toml file.
To make it right, I would remove the test_search_config_** tests and keep only the test_patched_read_toml_opts** versions, but with all these scenarios (possibly using a tempdir to set things up):
- Only
.mdformat.toml→ Options from.mdformat.tomlare returned - Only
pyproject.tomlwith options → No options are returned - Only
pyproject.tomlwithout options → No options are returned - Both
.mformat.tomlandpyproject.toml, butpyproject.tomlhas no options → Options from.mdformat.tomlare returned - Both
.mformat.tomlandpyproject.tomlexist and with different options → There is a warning (mock.patchtheprint_paragraphsfunction to certify it) and the options from.mdformat.tomlare returned.
I'm happy to make these changes myself, either on top of the same PR or after merging it, if you want, or you can give it a go. Your call.
In any case, I'll make the release with the rest to unblock the compatibility problem, and we can re-release afterwards with this one.
|
I can make the test changes, but not today. 😅 So if you are in a hurry do not hesitate to add them on top of this PR! |
.mdformat.toml takes precedence.mdformat.toml take precedence
Alright, I'll do the changes myself and release a new version with this possibly today. |
|
Thanks @csala for the additional tests and merge! |
Thank YOU for moving this in the first place! Good collaboration altogether. |
This PR closes #17
Additionally, fixes some docstring rST formatting.