-
-
Notifications
You must be signed in to change notification settings - Fork 165
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
[FIX] duplicate-xml-fields: False red using 2 tree sub-views *2M fields #96
[FIX] duplicate-xml-fields: False red using 2 tree sub-views *2M fields #96
Conversation
exclude = list(set(parents) - set(same_parent)) | ||
for item in items: | ||
if item.getparent() in exclude: | ||
items.remove(item) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pedrobaeza
Here we will have a issue with this remove, right?
cc @hugho-ad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hugho-ad
Please, review the following good comments from @pedrobaeza and fix them
#63 (comment)
We have the same case here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you have fixed it, so no problem now 😉
</form> | ||
</field> | ||
</record> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hugho-ad
Could you add the following test case?
diff --git a/pylint_odoo/test_repo/broken_module/model_view_odoo2.xml b/pylint_odoo/test_repo/broken_module/model_view_odoo2.xml
index 31d9e57..08c1b34 100644
--- a/pylint_odoo/test_repo/broken_module/model_view_odoo2.xml
+++ b/pylint_odoo/test_repo/broken_module/model_view_odoo2.xml
@@ -104,6 +104,28 @@
</form>
</field>
</record>
+ <!-- double duplicate record field "name" in *2M field-->
+ <record id="view_model_form6" model="ir.ui.view">
+ <field name="name">view.model.form</field>
+ <field name="model">test.model</field>
+ <field name="arch" type="xml">
+ <form string="Test model6">
+ <field name="picking_ids">
+ <tree>
+ <field name="name"/>
+ <field name="name"/>
+ </tree>
+ </field>
+ <field name="value_ids">
+ <tree>
+ <field name="name"/>
+ <field name="name"/>
+ </tree>
+ </field>
+ </form>
+ </field>
+ </record>
+
</data>
</odoo>
Here we should have 2 new messages emitted (increase the expected checkers)
- 'duplicate-xml-fields': 6,
+ 'duplicate-xml-fields': 8,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hugho-ad
FYI I ran the test and duplicated-xml-fields was emitted 7 times.
Maybe we need add the parent in the key in order to group all items, I mean:
- all_fields.setdefault(field_xml, []).append(field)
+ all_fields.setdefault((field_xml, field.getparent()), []).append(field)
And process even this multi key in all other part of code where is used.
cc @YannickB - this will fix the duplicate xml ID lint in Clouder 😄 |
@moylop260 increased the expected checkers, and travis is green |
@moylop260 please give your feadback here Regards |
parents.setdefault( | ||
parent, {}).setdefault((field_xml, parent), []).append(field) | ||
all_fields.setdefault((field_xml, parent), []).append(field) | ||
return parents, all_fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need 2 dictionaries to detect duplicated.
I mean, you could use just 1 dictionary since that you have the complex key:
- field_xml <- str
- parent <- node object (this is different although name string of parent is equal)
And the appended values are fields
Then you could detect them with
duplicated = [(field_xml, nodes) for (field_xml, parent), nodes in all_fields.items() if len(nodes) >= 2)
f5cd958
to
ccf20dd
Compare
@moylop260 comments Done! thks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
Feedback is welcome team |
ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woot thanks @moylop260
👍 |
Fix #76