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

[FIX] pylint-odoo: modify dangerous-view-replace-wo-priority to check all child tags with replace #309

Merged

Conversation

fernandahf
Copy link
Contributor

@fernandahf fernandahf commented Dec 22, 2020

Description of the issue/feature this PR addresses:

Consider next case:

<record id="crm_case_form_view_leads_form_inherit" model="ir.ui.view">
    <field name="name">crm.lead.form.lead</field>
    <field name="model">crm.lead</field>
    <field name="inherit_id" ref="crm.crm_case_form_view_leads"/>
    <field name="arch" type="xml">
        <label for="contact_name" position="replace"/>
        ...
    </field>
</record>

Part of code where replace is use is a label tag and pylint-odoo does not recognize it, however, Odoo convert this part of code in a xpath tag with replace attribute but in that point already passed lint.

Current behavior before PR: warnings of code like above not found.

Desired behavior after PR is merged: warnings of code like above found.

-- I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@fernandahf fernandahf force-pushed the master-review-dangerous-view-wo-fer branch from d3983d6 to 0112714 Compare December 22, 2020 20:27
@fernandahf
Copy link
Contributor Author

@luisg123v

Could you review this, please?

Regards.

Copy link
Contributor

@luisg123v luisg123v left a comment

Choose a reason for hiding this comment

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

Instead of the image, I'd paste the code in the description.

pylint_odoo/checkers/modules_odoo.py Outdated Show resolved Hide resolved
@fernandahf fernandahf force-pushed the master-review-dangerous-view-wo-fer branch 2 times, most recently from f8dd84b to e13b9f2 Compare December 23, 2020 21:49
@fernandahf
Copy link
Contributor Author

Instead of the image, I'd paste the code in the description.

Done

@fernandahf
Copy link
Contributor Author

@luisg123v

I think that its ready.

Copy link
Contributor

@luisg123v luisg123v left a comment

Choose a reason for hiding this comment

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

Hi @fernandahf,

Remember commit description is as (and sometimes more) as important than the PR description.

Could you copy PR description in the commit description, please?

@@ -88,7 +88,7 @@
</form>
</field>
</record>
<!-- Replace with low priority eval-->
<!-- -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, sorry, was an error.

pylint_odoo/test_repo/broken_module/model_view2.xml Outdated Show resolved Hide resolved
… all child tags with replace

Consider next case:

<record id=crm_case_form_view_leads_form_inherit model=ir.ui.view>
    <field name=name>crm.lead.form.lead</field>
    <field name=model>crm.lead</field>
    <field name=inherit_id ref=crm.crm_case_form_view_leads/>
    <field name=arch type=xml>
        <label for=contact_name position=replace/>
        ...
    </field>
</record>

Part of code where replace is use is a label tag and pylint-odoo does not recognize it, however, Odoo convert this part of code in a xpath tag with replace attribute but in that point already passed lint.
@fernandahf fernandahf force-pushed the master-review-dangerous-view-wo-fer branch from e13b9f2 to 1fc84f5 Compare December 23, 2020 23:07
@fernandahf
Copy link
Contributor Author

@luisg123v

Could you review this again, please?

Regards.

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.

3 participants