macisamuele / language-formatters-pre-commit-hooks

Collection of custom pre-commit hooks.
Apache License 2.0
116 stars 58 forks source link

Update pretty-format-ini to use `config_formatter` library #113

Closed Delgan closed 2 years ago

Delgan commented 2 years ago

Hi @macisamuele, thanks for sharing and maintaining these different pre-commit hooks!

While installing pretty-format-ini I noticed it did not format my .ini files "as expected". After some research I came across #99, #100 and #106. Seeing that iniparse and configobj libraries both had some unfortunate limitations, I decided to create my own formatting library: config_formatter (what a name!).

It's very simple and is based on the configupdater library which supports comments and is pretty well maintained.

What would you think of using config_formatter as part of your pretty-format-ini hook? I wrote it basically for the sole purpose of fixing #106 issues, so if there is any change to the API that you would like to see implemented, let me know and I will update the package according to your preferences. ;)

codecov[bot] commented 2 years ago

Codecov Report

Merging #113 (7af731d) into maci-fix-ini-tool-regression (9b73ca2) will not change coverage. The diff coverage is 100.00%.

:exclamation: Current head 7af731d differs from pull request most recent head 1ee1b17. Consider uploading reports for the commit 1ee1b17 to get more accurate results

@@                      Coverage Diff                       @@
##           maci-fix-ini-tool-regression      #113   +/-   ##
==============================================================
  Coverage                        100.00%   100.00%           
==============================================================
  Files                                10        10           
  Lines                               316       311    -5     
==============================================================
- Hits                                316       311    -5     
Impacted Files Coverage Δ
...e_formatters_pre_commit_hooks/pretty_format_ini.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9b73ca2...1ee1b17. Read the comment docs.

macisamuele commented 2 years ago

Thanks for doing this and for filling up the gaps on ini round-trip parsing :) Really appreciated

Give me some little time to play a bit with it in order to check that the most important parts are preserved. Mainly thinking to indentation, comments, types, etc.

I do see that the library documentation does already show some of what I'm looking for ... so I don't think that this check will take a long time.

FYI: I've enabled tests for this PR and I see that at very least pre-commits are reporting some issues, please make sure to address them

Delgan commented 2 years ago

Hey @macisamuele, thanks for the fast feedback!

I updated config_formatter to 1.1.0 and it now supports Python 3.6.

I also fixed pre-commit hooks: to do so I had to remove the 3.9 requirement as it was raising an exception on my dev environment (I'm using Python 3.10 on clean virtual environment):

[INFO] Installing environment for https://github.com/pre-commit/pre-commit.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command: ('/home/delgan/Programmation/language-formatters-pre-commit-hooks/env/bin/python', '-mvirtualenv', '/home/delgan/.cache/pre-commit/repoly7vpo7k/py_env-python3.9', '-p', 'python3.9')
return code: 1
expected return code: 0
stdout:
    RuntimeError: failed to find interpreter for Builtin discover of python_spec='python3.9'

stderr: (none)
Check the log at /home/delgan/.cache/pre-commit/pre-commit.log

I also had to update pinned black version as it generated an exception (inside the Github Actions as well):

black....................................................................Failed
- hook id: black
- exit code: 1

Traceback (most recent call last):
  File "/home/delgan/.cache/pre-commit/repov_ptxqqq/py_env-python3/bin/black", line 8, in <module>
    sys.exit(patched_main())
  File "/home/delgan/.cache/pre-commit/repov_ptxqqq/py_env-python3/lib/python3.10/site-packages/black/__init__.py", line 1129, in patched_main
    patch_click()
  File "/home/delgan/.cache/pre-commit/repov_ptxqqq/py_env-python3/lib/python3.10/site-packages/black/__init__.py", line 1115, in patch_click
    from click import _unicodefun  # type: ignore
ImportError: cannot import name '_unicodefun' from 'click' (/home/delgan/.cache/pre-commit/repov_ptxqqq/py_env-python3/lib/python3.10/site-packages/click/__init__.py)

It looks like it's a known issue with click: ImportError: cannot import name '_unicodefun' from 'click'.

Finally, I also had to update the target-version in .black.toml as 2.7 is no longer supported (by black and by this repository as well if I'm not mistaken).

macisamuele commented 2 years ago

@Delgan thanks a lot for your contribution. I've played a bit with config_formatter and I have to congratulate with you 😎 I haven't spot, yet, specific edge-cases that would prevent the switch to it.

Only one note for this PR in order to make it mergable I would advice to remove the changes related to pre-commit hooks and versions (they are carried over #114).

I would recommend to rebase this PR on top of ee7c7da. Once that is done and tests continue to be green this could be merged and released.

Delgan commented 2 years ago

Thanks @macisamuele! All the hard work is done in configupdater library actually. Hopefully end users won't notice any edge-case bug. :grin: In any case don't hesitate to ping me and I'll likely update the formatter if needed.

I reverted the changes related to pre-commit hooks. Note that my PR was initially based on your maci-fix-ini-tool-regression branch. I updated the destination toward master, consequently this PR includes your old commit. I can just merge the two added unit test files in one commit if you prefer.