OCA / odoo-pre-commit-hooks

Linters of Odoo addons that complement pylint-odoo
GNU Affero General Public License v3.0
10 stars 12 forks source link

Add pretty-format-po check #76

Closed antonag32 closed 1 year ago

antonag32 commented 1 year ago

Related to https://github.com/Vauxoo/pre-commit-vauxoo/issues/104

This new check ensures that all files follow a certain standard with message ids sorted alphabetically and lines wrapped at a width of 78 columns.

moylop260 commented 1 year ago

@luisg123v could you check it, please?

antonag32 commented 1 year ago

Working in the autofix part, should be ready soon

antonag32 commented 1 year ago

Need to fix the paths on windows. This new check can be tested with:

pip install git+https://github.com/vauxoo-dev/odoo-pre-commit-hooks.git@pretty-format-po

The reference file suggested by @luisg123v still fails this new lint, even when no wrapwidth is specified (default is now used) :thinking:

antonag32 commented 1 year ago

@moylop260 @luisg123v

I have corrected the mistake and added extra functionality to the hook (set msgstr to "" if it is the same as msgid).

As for the errors on windows, I think it has to do with tmpdir, see: https://bugs.python.org/issue7195

For now I skipped the tests on windows since if we wanted to fix this we would need to safely handle relative path calls (of which there are quite a few), however I do not know if it is worth it considering it probably won't happen outside of the tests

yajo commented 1 year ago

Wouldn't it be more helpful to use something like https://pypi.org/project/pre-commit-po-hooks/?

antonag32 commented 1 year ago

Wouldn't it be more helpful to use something like https://pypi.org/project/pre-commit-po-hooks/?

I checked the hooks and from what I see they don't perform any of the tasks this hook does, plus they don't have autofix support. Could you explain further?

moylop260 commented 1 year ago

Could you add in the readme a example to use this hook with autofix, please?

        - id: oca-checks-odoo-module
        - id: oca-checks-po
           - ....
moylop260 commented 1 year ago

For record,

I just tested this repo using the following pre-commit-config.yaml

...
  - repo: https://github.com/vauxoo-dev/odoo-pre-commit-hooks
    rev: pretty-format-po
    hooks:
      - id: oca-checks-po
        args:
          - --autofix

And running the following command:

The result was:

****po-pretty-format****
MODULE/i18n/fr.po is not formatted correctly - [po-pretty-format]
Fixing MODULE/i18n/fr.po ⚒

So, it is good to me!

moylop260 commented 1 year ago

Need to fix the paths on windows. This new check can be tested with:

pip install git+https://github.com/vauxoo-dev/odoo-pre-commit-hooks.git@pretty-format-po

The reference file suggested by @luisg123v still fails this new lint, even when no wrapwidth is specified (default is now used) 🤔

For record, I just tested the feature and the file referenced was not autofixed but i18n_extra/es.po

So, IMHO it is good

cc @luisg123v

antonag32 commented 1 year ago

Made the requested changes (README and arg name) @moylop260

luisg123v commented 1 year ago

Typo in reformatting commit:

"which was a instance variable itself" -> an instance

@antonag32 this is not fixed yet.

antonag32 commented 1 year ago

I corrected the two observations @luisg123v

moylop260 commented 1 year ago

@luisg123v

Could you re-check it, please?

moylop260 commented 1 year ago

@yajo

Wouldn't it be more helpful to use something like https://pypi.org/project/pre-commit-po-hooks/?

Could you check if the reply is good for you, please?

yajo commented 1 year ago

Yes, it's good. I just was giving one idea to avoid duplicating efforts, but if the hooks behave differently it's perfectly OK.

luisg123v commented 1 year ago

@antonag32

When I said:

"which was a instance variable itself" -> an instance

I meant changing "a" by "an", not removing "which was ... itself", but I see you removed it. Was that intentional or did I explain myself incorrectly?

antonag32 commented 1 year ago

@antonag32

When I said:

"which was a instance variable itself" -> an instance

I meant changing "a" by "an", not removing "which was ... itself", but I see you removed it. Was that intentional or did I explain myself incorrectly?

I misunderstood, fixed it now.

OCA-git-bot commented 1 year ago

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

moylop260 commented 1 year ago

/ocabot merge nobump

OCA-git-bot commented 1 year ago

What a great day to merge this nice PR. Let's do it! Prepared branch main-ocabot-merge-pr-76-by-moylop260-bump-nobump, awaiting test results.