Skip to content
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

[IMP] allow fix_manifest_website.py to fix only specified manifest files #522

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pasculorente
Copy link

@pasculorente pasculorente commented Jan 30, 2022

Fixes / new features

Please, feel free to use this PR as is or as an issue

I have read and followed the OCA guidelines

  • fix_manifest_website.py gets a new variadic argument [manifest]
  • if the argument is specified, only these manifests are fixed

The target of this PR is to improve the experience with the pre-commit hook. Right now, fix_manifest_website.py modifies all manifests under addons_dir. However, when using pre-commit, the expected behavior is that only files part of the commit are modified. Since pre-commit passes the list of files as a variadic argument, we can easily capture this list.

This PR delegates the filtering of the manifest files to the hook, expecting a configuration of the hook similar to:

- repo: https://github.com/oca/maintainer-tools
    rev: main
    hooks:
      - id: oca-fix-manifest-website
        args: ["https://www.odoo.com"]
        files: /__manifest__\.py$

Luckily, this configuration will call the script with the arguments: --addons_dir . https://odoo.com module_a/__manifest__.py

- fix_manifest_website.py gets a new variadic argument [manifest]
- if the argument is specified, only these manifests are fixed
@pasculorente pasculorente marked this pull request as ready for review January 31, 2022 16:51
@pasculorente
Copy link
Author

Hello @pedrobaeza how are you? who can I ping for this kind of tasks? Thank you

@pedrobaeza
Copy link
Member

Hi, Pascual,

It's difficult, as the maintainers of these tools are very few. Specifically this one, it has been contributed only by @sbidoul . Maybe he can take a look, but I'm not sure if it's intended that this check (and automatic change) is intended for the whole repo.

P.S.: I'm curious. You have put in your GH profile that your organization is Odoo S.A. Are you really working there? Is Odoo S.A. interested in these tools?

@pasculorente
Copy link
Author

Hi @pedrobaeza Thanks for your quick reply. Yes, in our development pipeline we are using this specific script as a pre-commit hook. It's very handy :)

@pedrobaeza
Copy link
Member

That's cool! I hope more synergies can be set between Odoo SA and OCA in the tooling.

@pedrobaeza
Copy link
Member

In which team do you work?

@sbidoul
Copy link
Member

sbidoul commented Apr 27, 2022

Hi @pasculorente thanks for the contrib. Generally LGTM, just a couple of minor usability remarks.

@pasculorente
Copy link
Author

Thanks @sbidoul . @pedrobaeza I'm part of the Professional Service Team in Odoo. I'll gladly contribute to this tools, however, I can only do it as a personal contribution.

@sbidoul
Copy link
Member

sbidoul commented Apr 27, 2022

Thanks @sbidoul . @pedrobaeza I'm part of the Professional Service Team in Odoo. I'll gladly contribute to this tools, however, I can only do it as a personal contribution.

Eh eh, want me to talk to your boss to explain the value of contributing back to open source projects 😉 ?

Thanks for contributing in any case!

@pedrobaeza
Copy link
Member

Generally LGTM, just a couple of minor usability remarks.

Where are the remarks?

@@ -12,36 +13,53 @@

@click.command()
@click.argument("url")
@click.argument("manifest", nargs=-1, type=click.Path())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@click.argument("manifest", nargs=-1, type=click.Path())
@click.argument("manifest", nargs=-1, type=click.Path(exists=True))

@@ -12,36 +13,53 @@

@click.command()
@click.argument("url")
@click.argument("manifest", nargs=-1, type=click.Path())
@click.option("--addons-dir", default=".")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have some help text saying that --addons-dir and manifests are mutually exclusive, and raise an error rather than ignoring silently.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sbidoul your comments are helpful. I will address them later and ping you back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants