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

Check attribute without translation parameter (OCA#104) #105

Conversation

JesusZapata
Copy link

Check attribute without translation parameter
Referring to OCA#104

@JesusZapata
Copy link
Author

@moylop260
Check if this meets the required?

self.msg_args = []
for xml_file in self.filter_files_ext('xml', relpath=True):
doc = self.parse_xml(os.path.join(self.module_path, xml_file))
if type(doc) is lxml.etree._ElementTree:

Choose a reason for hiding this comment

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

Use isinstance

Choose a reason for hiding this comment

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

...or What about use a sentence similar to https://github.com/Vauxoo/pylint-odoo/blob/9762664/pylint_odoo/checkers/modules_odoo.py#L735-L737?

            doc = self.parse_xml(os.path.join(self.module_path, xml_file))
            openerp_nodes = doc.xpath("/openerp") \
                if not isinstance(doc, basestring) else []

Copy link
Author

Choose a reason for hiding this comment

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

Done!
I have made improvements and optimized the process

for xml_file in self.filter_files_ext('xml', relpath=True):
doc = self.parse_xml(os.path.join(self.module_path, xml_file))
if type(doc) is lxml.etree._ElementTree:
for record in doc.xpath("//attribute[not(@translation)]"):
Copy link

@moylop260 moylop260 Jan 14, 2017

Choose a reason for hiding this comment

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

Could you add record to xpath too in order to get more precision?

Copy link
Author

Choose a reason for hiding this comment

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

Done!
Thanks for your comments!

def test_90_xml_attribute_translatable(self):
extra_params = ['--disable=all',
'--enable=xml-attribute-translatable']
pylint_res = self.run_pylint(self.paths_modules, extra_params)

Choose a reason for hiding this comment

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

Is really required this change?
I mean, this is tested from first test, Right?

Copy link
Author

Choose a reason for hiding this comment

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

One more time! Do you have reason!
I had included it because the test_25_checks_without_coverage method asked me to have a test, but it is no longer necessary!

for xml_file in self.filter_files_ext('xml', relpath=True):
doc = self.parse_xml(os.path.join(self.module_path, xml_file))
records = doc.xpath(
"/openerp//record//attribute[not(@translation)]") + \

Choose a reason for hiding this comment

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

I missed that we have pylint_odoo.misc.get_xml_records methods and you could re-use it here

Copy link
Author

Choose a reason for hiding this comment

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

It is much more direct because you are looking for only the attributes.
The get_xml_records method brings all the records and would have to search for them.
It's just for performance!

Copy link
Author

Choose a reason for hiding this comment

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

But you could append a parameter to the get_xml_records method to make it more direct!
Like this:

def get_xml_records(self, xml_file, model=None, more=''):                   
    .
    .
    .
    return doc.xpath("/openerp//record" + model_filter + more) + \          
    doc.xpath("/odoo//record" + model_filter + more) \                                                                                                     
    if not isinstance(doc, basestring) else [] 
    .
    .
    . 

The parameter is appended more!
more = '//attribute[not(@translation)]'

Copy link
Author

Choose a reason for hiding this comment

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

We add more functionality to the get_xml_records method.
I made the changes give me your opinion!

@moylop260
Copy link

Good choice man
Could you fix the red of travis
./pylint_odoo/checkers/modules_odoo.py:829:5: E125 continuation line with same indent as next logical line ?
and create a PR for OCA too

@JesusZapata
Copy link
Author

JesusZapata commented Jan 14, 2017

Done! Thanks!
De PR on OCA#105 based on the SHA bfb9de4

…#104)

[FIX] pylint-odoo: Fix pep8

[FIX] pylint-odoo: Improving conditions and optimizing

[FIX] pylint-odoo: Delete unnecessary test method

[IMP] pylint-odoo: New parameter to method get_xml_records

[FIX] pylint-odoo: Fix pep8 standar
@JesusZapata JesusZapata force-pushed the master-xml-attribute-translatable-jesuszapata branch from 573ff5b to bfb9de4 Compare January 14, 2017 12:30
@moylop260 moylop260 merged commit 0c9c870 into Vauxoo:master Jan 16, 2017
@moylop260 moylop260 deleted the master-xml-attribute-translatable-jesuszapata branch January 16, 2017 19:05
JesusZapata pushed a commit to vauxoo-dev/pylint-odoo that referenced this pull request Jan 26, 2017
JesusZapata pushed a commit to vauxoo-dev/pylint-odoo that referenced this pull request Jan 31, 2017
[FIX] pylint-odoo: Best use of if clause

[FIX] pylint-odoo: Validate the body contain only one line

[FIX] pylint-odoo: Avoid use \ better use (

[REF] incoherent-interpreter-exec-perm: Better message (Vauxoo#106)

[ADD] xml-attribute-translatable: Check XML attribute without translation parameter (Vauxoo#105)

Close OCA#104

[REF] javascript-lint: Use eslint instead of jshint (Vauxoo#97)

[ADD] renamed-field-parameter: Detect deprecated field values (digits_compute, select) (Vauxoo#99)

[FIX] Pep8 check and bad index for ODOO_MSGS

[ADD] attribute-string-redundant: Check if "string" parameter is equal to variable name (Vauxoo#100)

[FIX] attribute-string-redundant: Add "isinstance" validation for nodes

Fix OCA#109

[IMP] Exclude exception when use as assignation

[FIX] Pep8 check local variable 'exception'

[FIX] Pep8 blank line at end of file

[FIX] Adding more cases of test

[REF] Supporting more cases of exception

[FIX] Modify file main.py for adding new case of except-pass

[IMP] Adding another tests case

[FIX] Adding another test case and better validation

[FIX] Delete unnecessary condition of if

[FIX] Fix comment line
JesusZapata pushed a commit to vauxoo-dev/pylint-odoo that referenced this pull request Jan 31, 2017
[FIX] pylint-odoo: Best use of if clause

[FIX] pylint-odoo: Validate the body contain only one line

[FIX] pylint-odoo: Avoid use \ better use (

[REF] incoherent-interpreter-exec-perm: Better message (Vauxoo#106)

[ADD] xml-attribute-translatable: Check XML attribute without translation parameter (Vauxoo#105)

Close OCA#104

[REF] javascript-lint: Use eslint instead of jshint (Vauxoo#97)

[ADD] renamed-field-parameter: Detect deprecated field values (digits_compute, select) (Vauxoo#99)

[FIX] Pep8 check and bad index for ODOO_MSGS

[ADD] attribute-string-redundant: Check if "string" parameter is equal to variable name (Vauxoo#100)

[FIX] attribute-string-redundant: Add "isinstance" validation for nodes

Fix OCA#109

[IMP] Exclude exception when use as assignation

[FIX] Pep8 check local variable 'exception'

[FIX] Pep8 blank line at end of file

[FIX] Adding more cases of test

[REF] Supporting more cases of exception

[FIX] Modify file main.py for adding new case of except-pass

[IMP] Adding another tests case

[FIX] Adding another test case and better validation

[FIX] Delete unnecessary condition of if

[FIX] Fix comment line
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.

2 participants