[RFC] new check: module of inherited views must be in manifest depends key

[sbidoul]

For this typical view, the linter would check other_module is explicitly present in the depends key of the manifest.

This would catch many common dependency issues, and would enforce the best practice of explicitly declaring direct dependencies.

    <record id="view_..." model="ir.ui.view">
        <field name="name">my_view</field>
        <field name="model">my.model</field>
        <field name="inherit_id" ref="other_module.base_view" />
        <field name="arch" type="xml">
             ...
        </field>
    </record>

[pedrobaeza]

This will force to explicitly declared all possible dependency chain in the manifest. Example, you depend on sale_stock, and inherits one view of sale. It's not needed because sale_stock depends on sale, but you will be forced to put it. It's not critical, but it's an annoyance, specially with existing modules.

[moylop260]

@sbidoul it is a good idea but the sad part is that pylint-odoo is a static checker I mean pylint-odoo doesn't have full context of a full environment running or all modules used.

However, I have created a dynamic checker to detect this case of errors. https://github.com/Vauxoo/server-tools/tree/11.0/odoolint

I have detected the following common errors:

• Demo xml_id referenced from data xml (if the module is installed with --without-demo=all then it will fail)
• Unachievable xml_id if the module is installed with -i module (it is your case)
• Broken button If the module use a button action but is not defined on odoo
• Superfluous dependency:
  • sale_project: ['sale, 'project']
  • my_module: ['sale', 'base', 'sale_project', 'project']
  • Then sale, project and base modules are superfluous in my_module because of sale_project contains them. It last save a lot of time for -u process because of odoo runs many times the update of the same module for this case.

We can install this module from MQT for all builds and run this dynamic lints where the full environment is present.

If it is a good idea for you, just tell me what is the best repository to move it.

[sbidoul]

@moylop260 thanks for the feedback and the pointer to odoolint. I'll have a look.

My point here is that such a check is feasible statically if you force declaring all direct dependencies in the manifest, which is IMO a good practice anyway.

[pedrobaeza]

@sbidoul I prefer to not force that, as dependency chain changes a lot over versions (some examples: auth_crypt, sale_stock, sale_order_dates, account_invoicing, ...), so migrating a module is way more difficult with that decision or the risk of installing more things than needed is high.

[sbidoul]

On the contrary I find it easier to migrate when direct dependencies are explicitly listed in the manifest. Especially when the dependencies change it's good to know explicitly which are the required dependencies instead of having to go through all the code to find them.

[pedrobaeza]

I don't think that way based in my experience, but as always, we can do a survey for choosing the preferred way among all contributors. Any way, we can't put this as mandatory on existing branches for not breaking all of them, but only on new ones (13.0).

[voronind]

@sbidoul I prefer to not force that, as dependency chain changes a lot over versions (some examples: auth_crypt, sale_stock, sale_order_dates, account_invoicing, ...), so migrating a module is way more difficult with that decision or the risk of installing more things than needed is high.

How does absent sale dependency help you if you wrote in your code: ref="sale.some"? When you migrate module you refactor dependencies too.

[moylop260]

it did not have much approval from everyone apparently