[IMP] add manifest-external-assets check #525
Conversation
antonag32
force-pushed
the
manifest-external-assets-anton
branch
from
948aa7e to
6ed2266
Compare
|
Can you review @moylop260 @luisg123v? |
| if isinstance(element, nodes.Const): | ||
| if is_external_url(element.value): | ||
| self.add_message("manifest-external-assets", node=element) | ||
| elif isinstance(element, nodes.Tuple): |
There was a problem hiding this comment.
Notice it could be a Tuple or a List
See the following example:
| elif isinstance(element, nodes.Tuple): | |
| elif isinstance(element, (nodes.Tuple, nodes.List)): |
There was a problem hiding this comment.
In fact, Using manifest_get you could get all the isinstance(..., iterable) object if it is not a string and call it recursively
There was a problem hiding this comment.
I was checking if it was a tuple because of this use case:
Odoo docs always show this usage with tuples but I guess in theory you could also use a List although I have not seen it before, I will add it so we cover all possibilities.
| if item[0].value == "assets": | ||
| assets_node = item[1] | ||
|
|
||
| # it is important to use the actual astroid.node instead of manifest_dict, otherwise the |
There was a problem hiding this comment.
I would prefer to use manifest_dict + args in order to use the string value with the problem
I mean,
Currenly it is good showing the number of line:
twelve_module/__manifest__.py:15:12: {msg}
twelve_module/__manifest__.py:19:12: {msg}
But it could be good too using
twelve_module/__manifest__.py:12:14: {msg} {wrong url}
twelve_module/__manifest__.py:12:14: {msg} {wrong url}
So, the result final could be:
twelve_module/__manifest__.py:12:14: W8162: Assets https://shady.cdn.com/somefile.js should be distributed with module's source code. More info at https://httptoolkit.com/blog/public-cdn-risks/ (manifest-external-assets)
twelve_module/__manifest__.py:12:14: W8162: Assets https://bad.idea.com/cool.css should be distributed with module's source code. More info at https://httptoolkit.com/blog/public-cdn-risks/ (manifest-external-assets)It should be good since that manifest_dict is more pythonistic than using astroid nodes
So, it is easy to maintain
Only consider a recent fix that I have added for values wrong formed
There was a problem hiding this comment.
I originally tried using manifest_dict but had issues with test_180 which runs parallel jobs. It seems like message were being mangled up because all errors pointed to the same root node (the assets node), also the linenumber was wrong since we were pointing to assets node, instead of the actual offending URL.
There was a problem hiding this comment.
Good point for parallels!!!
I didn't know it
I have bypassed a few checks for parallels
Could you check if you can fix them (another PR)
There was a problem hiding this comment.
Forget it
I think it is a different matter
antonag32
force-pushed
the
manifest-external-assets-anton
branch
from
6ed2266 to
e7e9893
Compare
This new check verifies that no external sources are referenced on modules' assets. Related to: OCA/odoo-pre-commit-hooks#124.
antonag32
force-pushed
the
manifest-external-assets-anton
branch
from
e7e9893 to
d03a521
Compare
|
@moylop260 I improved the error message by including the actual URL that is offending and also added support for handling lists and not only tuples. I still left the astroid implementation because of the errors I previously found during unit testing, and also because even though I agree that it is uglier than manifest_dict, I think accurate line numbers are important when reporting errors. Otherwise it can be confusing, specially when there are lots of assets. |
Check release notes: - https://pypi.org/project/pylint-odoo v9.3.3 [FIX] prohibited-method-override: Check if caller has a func attribute (OCA/pylint-odoo#524) [ADD] deprecated-name-get: name_get deprecated since v17 (OCA/pylint-odoo#526) [ADD] manifest-external-assets: No external sources for assets (OCA/pylint-odoo#525)
Check release notes: - https://pypi.org/project/pylint-odoo/9.3.3 [FIX] prohibited-method-override: Check if caller has a func attribute (OCA/pylint-odoo#524) [ADD] deprecated-name-get: name_get deprecated since v17 (OCA/pylint-odoo#526) [ADD] manifest-external-assets: No external sources for assets (OCA/pylint-odoo#525)
This new check verifies that no external sources are referenced on modules' assets.
Related to: OCA/odoo-pre-commit-hooks#124.