Skip to content
This repository has been archived by the owner on May 16, 2024. It is now read-only.

linting field names? #385

Closed
hbrunn opened this issue Nov 16, 2016 · 11 comments
Closed

linting field names? #385

hbrunn opened this issue Nov 16, 2016 · 11 comments
Labels

Comments

@hbrunn
Copy link
Member

hbrunn commented Nov 16, 2016

In odoo/odoo#13780, a quite difficult to find bug was introduced by the fact that some javascript code breaks if a record has a column with the name length.

The specific instance where this was problematic is fixed in odoo core now, but I assume a lot of community addons will have a problem with this too. As it's probably not feasible to work around the problem in every module, should we introduce a lint test that forbids some field names? In this case, it would be length, but I can also think of a couple of other field names I'd like to forbid, like python keywords.

@lasley
Copy link
Contributor

lasley commented Nov 16, 2016

An excellent idea 👍

@moylop260
Copy link
Contributor

Thanks for report.

I didn't find the piece of js code with the problem.
Do you have a example of the code that we should avoid?

@hbrunn
Copy link
Member Author

hbrunn commented Nov 16, 2016

_.each, the rationale can be found here: http://underscorejs.org/#each

@lasley
Copy link
Contributor

lasley commented Nov 16, 2016

@moylop260 the issue is related specifically to the use of length as a field name. Certain views are unable to display the field due to the note linked by @hbrunn

Here's a solid sample: https://github.com/gdgellatly/odoo_bug_demos/search?utf8=%E2%9C%93&q=length
And Here's Odoo SA doing it in one of their modules: https://github.com/odoo/odoo/blob/fc2e80cb4bcc450762c7ac5cb82a3e2d88062b38/addons/delivery/models/product_packaging.py#L12

@moylop260
Copy link
Contributor

I got it
Thank you

Are there another fields name reserved for js?

@hbrunn
Copy link
Member Author

hbrunn commented Nov 17, 2016

I can't think of any right now. But I also couldn't think of length being problematic until I read this, so that doesn't say much...

@moylop260
Copy link
Contributor

Fix OCA/pylint-odoo#86

@hbrunn
Copy link
Member Author

hbrunn commented Nov 21, 2016

thanks!

@hbrunn hbrunn closed this as completed Nov 21, 2016
@moylop260
Copy link
Contributor

@hbrunn
Could you help us to add doc in our guidelines for this case?

@hbrunn
Copy link
Member Author

hbrunn commented Nov 21, 2016

I'd write something like

The property name `length` is used by some javascript libraries to determine if a reference points to an array or an object. As the availability of a member called `length` will break some javscript code, you shouldn't use this name at all

@gdgellatly
Copy link

Notwithstanding that length is already used as field name in odoo core on product model. Years I dealt with that issue.

pedrobaeza pushed a commit to OCA/pylint-odoo that referenced this issue Nov 26, 2016
* [REF] attribute-deprecated: Deprecate length class attribute
Related issue: OCA/maintainer-quality-tools#385 and odoo/odoo#13780

* [REF] .travis.yml: Disable F999

Disable the error: "dictionary key 'name' repeated with  different value"
This error is duplicated from pylint.
moylop260 added a commit to Vauxoo/pylint-odoo that referenced this issue Dec 9, 2016
* [REF] global refactoring: better message output and use real file and line number in non-py files (#62)

[REF] global refactoring: better message output and use real file and line number in non-py files

Change from `pylint_odoo/test_repo/broken_module/__init__:1: [W7910(wrong-tabs-instead-of-spaces), ]  model_view_odoo.xml:23 Use wrong tabs indentation instead of four spaces`
to `pylint_odoo/test_repo/broken_module/model_view_odoo.xml:23: [W7910(wrong-tabs-instead-of-spaces), ]  Use wrong tabs indentation instead of four spaces`

* [FIX] rst-syntax-error: Skip unknown directives

* [FIX] rst-syntax-error: Skip unknown roles

* [ADD] missing-import-error, missing-manifest-dependency

* [REF] README: Update messages list

Signed-off-by: Moisés López <moylop260@vauxoo.com>

* Adding isort dependency (#70)

* [REF] requirements: Update developer version of pylint and astroid

* [ADD] Support for 10.0 manifest name

* [REF] missing-import-error: Skip test file
since these files are loaded only when running tests and in such a case your module and their external dependencies are installed.

* [IMP] manifest-version-format: Add valid_odoo_versions parameter to force a valid version of odoo in the manifest version

* [FIX] missing-import-error: Updating libraries used from requirements.txt but not imported or nested imported from odoo

* [FIX] manifest-version-format: Support -e manifest-version-format only

* [FIX] manifest-version-format: Fix regex to use explicit dot instead of any char

* [FIX] Whitelist `anybox.testing.openerp`
* Add `anybox.testing.openerp` - Fixes #81

* [ADD] missing-return
If you use call a `super` method then you will need return the original
value.
If you want overwrite a original method then you need add documentation
of why and add a `pylint: disable=missing-return`

* [REF] attribute-deprecated: Deprecate length class attribute (#86)

* [REF] attribute-deprecated: Deprecate length class attribute
Related issue: OCA/maintainer-quality-tools#385 and odoo/odoo#13780

* [REF] .travis.yml: Disable F999

Disable the error: "dictionary key 'name' repeated with  different value"
This error is duplicated from pylint.

* [FIX] method-NAME: Fix case compute=None
Fix OCA#88
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants