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

[12.0] add pylint-odoo pre-commit hook #12

Merged
merged 1 commit into from
Oct 18, 2019

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented Oct 1, 2019

Add the default pylint config in .pylintrc.

The benefit of having it in the repo is that IDE can use it without any manual configuration.

still to be investigated

  • how to handle "beta" pylint-odoo messages from MQT

.travis.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

@sbidoul Thank you. that looks great.Why do you try to test with odoo 13?

@sbidoul sbidoul force-pushed the 12.0-pre-commit-pylint-sbi branch 3 times, most recently from 2f7c7a0 to 5975be2 Compare October 4, 2019 07:38
@sbidoul
Copy link
Member Author

sbidoul commented Oct 4, 2019

For beta pylint message my current idea is this:

  1. have .pylintrc with all messages enabled, so an IDE that picks it will show all warnings
  2. have .pylintrc-mandatory also present, with only mandatory checks
  3. have pre-commit run pylint with .pylintrc-mandatory
  4. have pre-commit run pylint a second time with .pylintrc and `--exit-zero', so pre-commit (and thus travis) will still show all beta messages

@sbidoul sbidoul force-pushed the 12.0-pre-commit-pylint-sbi branch 2 times, most recently from 85fcfb7 to 98d854a Compare October 6, 2019 11:50
@sbidoul
Copy link
Member Author

sbidoul commented Oct 7, 2019

Let's wait until the config is stabilized, then don't forget to squash. It's important to have all that in one commit.

@lmignon
Copy link
Contributor

lmignon commented Oct 7, 2019

@sbidoul Yes, I'm waiting. My changes are to fix the tests...

after_success: true
before_install: true
after_success:
before_install:
Copy link
Member Author

Choose a reason for hiding this comment

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

@lmignon FYI. This should clarify that after_success and before_install are overrides of the the global default, and not a boolean flag (it was actually running the true command).

@lmignon
Copy link
Contributor

lmignon commented Oct 16, 2019

/ocabot merge

@OCA-git-bot
Copy link
Contributor

Sorry @lmignon you are not allowed to merge.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@simahawk
Copy link
Contributor

@sbidoul can you merge this? 🙏

@sbidoul
Copy link
Member Author

sbidoul commented Oct 18, 2019

Yes of course. BTW, Only @sebastienbeau is part of this PSC. We should invite others.

/ocabot merge

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 12.0-ocabot-merge-pr-12-by-sbidoul-bump-no, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Oct 18, 2019
Signed-off-by sbidoul
@OCA-git-bot OCA-git-bot merged commit ec16228 into OCA:12.0 Oct 18, 2019
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 54fb3f1. Thanks a lot for contributing to OCA. ❤️

@lmignon lmignon deleted the 12.0-pre-commit-pylint-sbi branch October 18, 2019 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants