[IMP] pylint_odoo: Add check for manifest-version-format #18
Conversation
|
Hey @lescobarvx, thank you for your Pull Request. It looks like some users haven't signed our Contributor License Agreement, yet.
Appreciation of efforts, |
|
Could you run messages2rst method and modified the README.rst file with your new check, please? |
| @@ -8,7 +8,7 @@ | |||
| from pylint_odoo import misc | |||
|
|
|||
|
|
|||
| EXPECTED_ERRORS = 52 | |||
| EXPECTED_ERRORS = 54 | |||
There was a problem hiding this comment.
As commented other times, it would be better to add a line like this:
EXPECTED_ERRORS += 2 # Errors due to C8106: Wrong version format
This way, multiple PRs don't get affected each other and the number seems less random.
luiseevaquer
force-pushed
the
master-oca-add-pylint-check-manifest-ver-dev-lescobarvx
branch
2 times, most recently
from
1417f04
to
95891e1
Compare
|
Thanks! 👍 |
|
You're welcome!! |
|
👍 |
| @@ -261,6 +274,15 @@ def visit_dict(self, node): | |||
| self.add_message('license-allowed', | |||
| node=node, args=(license,)) | |||
|
|
|||
| # Check version format | |||
| version_format = manifest_dict.get('version', None) | |||
There was a problem hiding this comment.
Change the code:
- version_format = manifest_dict.get('version', None)
+ version_format = manifest_dict.get('version', '')
To avoid follow error:
luiseevaquer
force-pushed
the
master-oca-add-pylint-check-manifest-ver-dev-lescobarvx
branch
from
95891e1
to
6d2415b
Compare
|
@guewen @max3903 @dreispt @lepistone your feedback is welcome |
| @@ -9,6 +9,7 @@ | |||
|
|
|||
|
|
|||
| EXPECTED_ERRORS = 52 | |||
| EXPECTED_ERRORS += 2 # Errors due to C8106: Wrong version format | |||
There was a problem hiding this comment.
@lescobarvx why not EXPECTED_ERRORS = 54 in one line and add the comment in the commit message?
otherwise 👍
There was a problem hiding this comment.
@max3903
@pedrobaeza indicated me to do so because that way multiple PRs don't get affected each other and the number seems less random.
There was a problem hiding this comment.
@lescobarvx ok, thanks.
👍
There was a problem hiding this comment.
There was a problem hiding this comment.
Yeah, is more easy to follow the total number of expected errors and to avoid conflicts if multiple PRs are done simultaneously.
There was a problem hiding this comment.
@pedrobaeza What do you propose to avoid having this after couple contributions?
EXPECTED_ERRORS = 52
EXPECTED_ERRORS += 2 # Errors due to C8106: Wrong version format
EXPECTED_ERRORS += 3 # Errors due to XXXX
EXPECTED_ERRORS += 4 # Errors due to YYYY
EXPECTED_ERRORS += 5 # Errors due to ZZZZ
There was a problem hiding this comment.
:o) Ok, but at some point (when there is no active PR), we should reset that to a single line.
Let's merge it and move forward.
There was a problem hiding this comment.
Well, I prefer to let unfolded. As said, this allows to identify how many errors of each type you have introduced. If you have a global counter, you need to check each of them for a global number, and it's more prone to put the given number by the test than the real expected one.
|
CLA is OK. |
|
@moylop260, should we make a new version? |
|
Hey @lescobarvx,
Appreciation of efforts, |
Yes |
|
As Luis is proposing other changes, we can wait for them. |
luisg123v
deleted the
master-oca-add-pylint-check-manifest-ver-dev-lescobarvx
branch
[IMP] pylint_odoo: Add check for manifest-version-format