OCA/oca-addons-repo-template

Enable new checks for pylint core

moylop260 opened this issue · 14 comments

moylop260 commented

pylint core core has useful checks but they are disabled from OCA configuration files because we are only enabling new checks from pylint-odoo

e.g.

  • super-with-arguments - Consider using Python 3 style super() without arguments

Now, py3 doesn't require to define:

  • super(ClassName, self).method()

Only using

  • super().method()

So, IMHO it could be good idea enabled for new versions

I just cloned_everything and ran pre-commit-vauxoo overall projects in order to get the lints enabled filtered from my side but not from OCA

The result was the following checks:

moylop260 commented

Check the following example detected:

The code has:

 def _is_roulier(self):
        self.ensure_one()
        available_carrier_actions = roulier.get_carriers_action_available() or {}
        return "get_label" in available_carrier_actions.get(self.delivery_type, [])

def cancel_shipment(self, pickings):
        if self._is_roulier:
            pickings._cancel_shipment()
        else:
            return super().cancel_shipment(pickings)

Notice the if self._is_roulier: it will be True all the time since it is a method definition

It should be

-        if self._is_roulier:
+        if self._is_roulier():

So, this module will not working as expected but it could be detected from pylint checks

BTW This is a code 3 years old

cc @florian-dacosta

  • 👍1
moylop260 commented

Check the following case:

It is using str.format for a logger

It is better evaluating the string from the logger method itself it save processor if the logger is disabled and avoid raising errors if the format is wrong to show a logger

- _logger.error("Paynet contract for {} on {} not found".format(self.name, transmit_method.name))
+ _logger.error("Paynet contract for %s on %s not found, self.name, transmit_method.name)
  • 👎1
moylop260 commented

Check the following

- message = "O Documento Fiscal %s foi exportado." % inv.fiscal_number
+ self.message = "O Documento Fiscal %s foi exportado." % inv.fiscal_number  # or something similar
  • 👍1
moylop260 commented

Check the following case:

country_code = country_code

  • 👍1
moylop260 commented

Check the following case:

- new_subkpis = {subkpi for subkpi in self.subkpi_ids}
+ new_subkpis = set(self.subkpi_ids)
  • 👍1
moylop260 commented

Why is better using a lazy logger?

Notice the following case:

  • _logger.info("Wrong logger %s" % ("hello1", "hello2"))

Result:

  • TypeError: not all arguments converted during string formatting

But using:

  • _logger.info("Wrong logger %s", "hello1", "hello2")

It will not raises error

Note: odoo _logger is not compatible with "{}".format _logger.info('Hello, {}', 'world')

File "/usr/lib/python3.8/logging/init.py", line 373, in getMessage
msg = msg % self.args
TypeError: not all arguments converted during string formatting

yajo commented

I agree we should add those checks.

moylop260 commented

What file/section should be it added?

moylop260 commented

Hi @yajo @sbidoul

Could you take advantage to discuss all these lints during OCA days and make a PR to enable them?

cc @max3903 @jjscarafia

yajo commented

I won't be there this year, sorry. Only odoo experience.

sbidoul commented

@moylop260 are you at OCA days? I've not seen you yet.

moylop260 commented

Hi my friends

This year we won't travel to Belgium

github-actions commented

There hasn't been any activity on this issue in the past 6 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this issue to never become stale, please ask a PSC member to apply the "no stale" label.