odoo / odoo

Odoo. Open Source Apps To Grow Your Business.
https://www.odoo.com
Other
37.95k stars 24.67k forks source link

Proposal: replacing __openerp__.py with __odoo__.yaml #6287

Closed dreispt closed 8 years ago

dreispt commented 9 years ago

It's planned to replace the __openerp__ name for the manifest file with __odoo__. I propose to use that opportunity to also change the type of file from .py to .yaml.

My argument is that the manifest is actually data, not code, so it can feel awkward to have it on a code file. A more pragmatic issue is that evaluating the __openerp__.py file to parse is an important security risk: it's easy to inject trojan code in a module and publish it on the App store. With YAML we could use the safer deserialization yaml.safe_load().

If I know it would be welcome, I 'm willing to propose a PR for this. (cc @antonylesuisse)

nbessi commented 9 years ago

:+1: for yaml or any looks alike format

bringsvor commented 9 years ago

Would it be possible to do both (odoo.py and odoo.yaml)? Am I the only one that don't really like YAML? It's syntax seem to be very fragile and the error reporting is very unspecific. Suggested topic for your next book Daniel: "Writing YAML tests for Odoo, and learning to like it...".

Torvald Baade Bringsvor Bringsvor Consulting AS - Odoo (formerly OpenERP) implementation partner

2015-04-13 10:38 GMT+02:00 Nicolas Bessi (nbessi) notifications@github.com :

[image: :+1:] for yaml or any looks alike format

— Reply to this email directly or view it on GitHub https://github.com/odoo/odoo/issues/6287#issuecomment-92271803.

xmo-odoo commented 9 years ago

A more pragmatic issue is that evaluating the __openerp__.py file to parse is an important security risk: it's easy to inject trojan code in a module and publish it on the App store.

ast.literal_eval is sufficient to fix that issue.

bealdav commented 9 years ago

As far as I know at mozilla, manifest file is named manifest.... Seems to be a good practice ?

clonedagain commented 9 years ago

:+1: XML or YAML would suit the purpose better than evaluated python strings, and woyld make it plain that you can't insert actual code in it.

adrian-nz commented 9 years ago

-1 unless there a valid reason ast.literal_eval won't fix the security problem.

Remembering tomcat/web.xml vs django/settings.py

On 14 April 2015 at 09:04, Lionel Sausin notifications@github.com wrote:

[image: :+1:] XML or YAML would suit the purpose better than evaluated python strings, and woyld make it plain that you can't insert actual code in it.

— Reply to this email directly or view it on GitHub https://github.com/odoo/odoo/issues/6287#issuecomment-92498224.

jsh-odoo commented 9 years ago

I personally think that putting manifest file in JSON format is also possible option. It is more readable comparing to XML. I don't think that dealing with JSON file will rise these kind of security problems but I am not sure. Thanks. Source : http://stackoverflow.com/questions/15529252/ensuring-safe-json-xml-and-yaml-loading-in-django-project

bringsvor commented 9 years ago

+1 for json. And +1 for manifest, maybe Odoo will change names again sometime..

Torvald

  1. apr. 2015 07.11 skrev "Jaysinh Shukla" notifications@github.com:

I personally think that putting manifest file in JSON format is also possible option. It is more readable comparing to XML. I don't think that dealing with JSON file will rise these kind of security problems but I am not sure. Thanks. Source : http://stackoverflow.com/questions/15529252/ensuring-safe-json-xml-and-yaml-loading-in-django-project

— Reply to this email directly or view it on GitHub https://github.com/odoo/odoo/issues/6287#issuecomment-92601481.

xmo-odoo commented 9 years ago

As far as I know at mozilla, manifest file is named manifest…. Seems to be a good practice ?

That's part of the considerations noted in #105

I personally think that putting manifest file in JSON format is also possible option. It is more readable comparing to XML. I don't think that dealing with JSON file will rise these kind of security problems but I am not sure. Thanks.

If a decision is made to switch manifests to literal_eval, there's limited reason to use JSON instead: save for custom decoders (or object_hook) Python literals are a superset of JSON, and the syntax is almost identical (if anything it's slightly more convenient as JSON does not support multiline or raw strings).

+1 for json. And +1 for manifest, maybe Odoo will change names again sometime..

The root library and manifest files have yet to be renamed following the Odoo switch, they could yet be.

nhomar commented 9 years ago

I like a generic name __manifest__. I prefer a .py like sphinx and django a dict is confusing sometimes and a yml is string also, a python file always gives more flexibility.

sebastienbeau commented 9 years ago

I really like the generic name "manifest".

Regarding json VS yaml VS .py. I am ok with all but we should only support one to avoid losing time with heterogeneous format.

bguillot commented 9 years ago

I agree with the name "manifest" and with yaml or json format.

legalsylvain commented 9 years ago

@dreispt : My argument is that the manifest is actually data

:+1: for non python format;

@bringsvor : maybe Odoo will change names again

:+1: for manifest name;

@sebastienbeau : I am ok with all but we should only support one to avoid losing time with heterogeneous format.

:100: :+1:

Note: Since V8, most of the description is written in another file, so manifest file will be more easy to write; (name, summary, author, version, files list;...)

My 2 cents.

sylvainc commented 9 years ago

:+1: for a generic name like "manifest" and a non-python format

florian-dacosta commented 9 years ago

:+1 for the name 'manifest' and for json/yaml/xml format

yvaucher commented 9 years ago

:+1: for a manifest file with generic name.

:+1: for yaml or json

I'd rather not use XML for manifest, it's less readable

antonylesuisse commented 9 years ago

We parse it with literal eval, if we change the syntax we would switch to json. odoo.json or manifest.json.

clonedagain commented 9 years ago

Out of curiosity, why JSON rather that YAML when we already have so many YAML data files (mostly automatic tests)?

antonylesuisse commented 9 years ago

Because YAML is complex and ugly and JSON simple and beautiful.

The full http://json.org/ spec fits on my screen, official rfc4627 and ecma404 are very terse too. Compare it with http://yaml.org/spec/1.2/spec.html

To be honest i never understood the syntax of yaml

parthivgls commented 9 years ago

+1 for JSON.

On Wed, Apr 15, 2015 at 10:52 PM, antonylesuisse notifications@github.com wrote:

Because YAML is complex and ugly and JSON simple and beautiful.

The full http://json.org/ spec fits on my screen, offcial rfc4627 and ecma-404 are very terse too. Compare it with http://yaml.org/spec/1.2/spec.html

— Reply to this email directly or view it on GitHub https://github.com/odoo/odoo/issues/6287#issuecomment-93492025.

Thanks & Regards, Parthiv Patel

clonedagain commented 9 years ago

OK but then you should altogether deprecate YAML support in Odoo and migrate the automatic tests to JSON. And maybe deprecate XML too. We don't need 3 data languages do we?

vaab commented 9 years ago

Ahah, let me add another useless stance: YAML is simple and beautiful and JSON is complex and ugly.

Just try one second to convert the YAML tests to JSON and you'll understand.

And it's the logical path to simplicity:

The size of the spec is mostly for advanced features that can be avoided. If you stay with struct, sequence, and literals... no issues, it's quite clear and pleasing to the eye.

Please avoid XML.

As xmo said, JSON has no value added against python syntatically (in fact, it's worse). The only added advantage with JSON is to open these file to a well known data format with many implementation on any language, which is what they are all about.

At last, using JSON while all the tests are in YAML, is adding a third data language.

And using a generic name as metadata.yml seems wise and prevent the endless renaming of files in the future. (__terp__.py -> __openerp__.py -> __odoo__.py -> ... ?)

Very nice overview of JSON vs Yaml from a former Yaml hater (same rightfull start about specs being so complex): http://www.cowtowncoder.com/blog/archives/2012/04/entry_473.html

guewen commented 9 years ago

I also think that json is too limited at least for the lack of comments and multiline.

I don't like Yaml, but at least it would support the needs.

Cargo (http://doc.crates.io/ the Rust's package manager) uses a relatively recent language: toml https://github.com/toml-lang/toml which is easy to read and to write.

nbessi commented 9 years ago

For manifest file I would prefer a MANIFEST.ini file: https://docs.python.org/3/library/configparser.html https://docs.python.org/2.7/library/configparser.html

It is supported in the Python standard lib with multi line support out of the box. That format is also easy to integrate with buildout or the setuptools/disutils tools.

It should also be of some use for a potential transition of Odoo addons into eggs.

My two cents

Nicolas.

vaab commented 9 years ago

python configparser's ini format is not a standard out of python community (as far as I know).

How am I supposed to make c/go/bash/javascript/ruby... scripts that read these ? Same applies for toml (yet another ini-like format).

Use case: I would like to be able to read the version of a module without having to write a python program for that. Or list dependencies (to draw a graph), write a linting software, write a module store... Using Yaml/Json/Xml has the advantage to open the content of the manifest to any language, thus greatly enhancing interoperability.

I'm curious, what had you in mind about integrating this file with buildout/setuptools tools ?

vaab commented 9 years ago

For those not familiar with YAML, this is the current hr/__openerp__.py converted to YAML:

name: Employee Directory
version: 1.1
author: OpenERP SA
category: Human Resources
sequence: 21
website: https://www.odoo.com/page/employees
summary: Jobs, Departments, Employees Details
description: |
  Human Resources Management
  ==========================

  This application enables you to manage important aspects of your 
  company's staff and other details such as their skills, contacts, working time...

  You can manage:
  ---------------
  * Employees and hierarchies : You can define your employee with User 
    and display hierarchies
  * HR Departments
  * HR Jobs
depends: 
  - base_setup
  - mail
  - resource
  - board
  - web_tip
data: 
  - security/hr_security.xml
  - security/ir.model.access.csv
  - hr_view.xml
  - hr_installer.xml
  - hr_data.xml
  - hr_tip_data.xml
  - res_config_view.xml
  - mail_hr_view.xml
  - res_users_view.xml
  - views/hr.xml
demo: 
  - hr_demo.xml
test: 
  - test/hr_users.yml
  - test/open2recruit2close_job.yml
  - test/hr_demo.yml
installable: True
application: True
auto_install: False
qweb: 
  - static/src/xml/suggestions.xml
aisopuro commented 8 years ago

:+1: for yaml and __manifest__.

osvalr commented 8 years ago

:+1: to having __manifest__.yml, take a look at @vaab example above, IMO is a more readable and straight forward approach for addon manifest definition

rossasa commented 8 years ago

+1 manifest.json because the format will be very similar to current format, and JSON it's a data format python it's for programming

dreispt commented 8 years ago

@rossasa JSON does not allow for comments, and that's a show stopper for this type of files.

rossasa commented 8 years ago

Yeah @dreispt I forgot that! I agrree with you, I'm used to comment some lines in openerppy for testing and it's very usefull

antonylesuisse commented 8 years ago

We plan to rename __openerp__.py -> __manifest__.py:

JSON pro:

JSON con:

Literal eval PY pro:

Literal eval PY con:

dreispt commented 8 years ago

@antonylesuisse Thanks for sharing. I guess that the last resisting mention to "openerp" will be the /openerp/ directory in the source...

antonylesuisse commented 8 years ago

We will rename /openerp/ -> /odoo/ soon.

menecio commented 8 years ago

@antonylesuisse not even a chance of considering Yaml? it can be very clean like the example of https://github.com/odoo/odoo/issues/6287#issuecomment-95879711

mlaitinen commented 8 years ago

I hope Odoo S.A. will hear OCA's opinion in this matter. If replacing Python manifest file completely with YAML is not possible, at least I'd like to have the option to choose between __manifest__.yml and __manifest__.py.

danidee10 commented 8 years ago

@mlaitinen what benefits would a YAML file provide over a python file

dreispt commented 8 years ago

@mlaitinen I must clarify that the OCA has not expressed any opinion about this. Comments here are on behalf of their authors.

aisopuro commented 8 years ago

The biggest advantage to me is readability and (human) parseability: putting data into Python produces a lot of cruft, since every string has to be surrounded by delimiters "", every list by brackets [] and so on. For me this is as much a philosophical argument as it is a practical one: the __openerp__.py file does not actually contain any code. It contains data, and nothing but data. "It's easier to use because it just works in Python" doesn't sound like a great reason: we don't allow the user to specify global configurations like addons_path or admin_passwd through .py-files either. The other Python pros mentioned by @antonylesuisse, multiline strings and comments, are also supported by YAML (which I favor).

I can absolutely see that this could cause some real headaches. I think @mlaitinen's idea is good for a transition period, so people don't have to rework their manifest files overnight. But configuration should be kept separate from runnable code in my opinion.

antonylesuisse commented 8 years ago

The spec of http://yaml.org/spec/1.2/spec.html is 800KB large it's way too complex. By this choice and as we removed all yml tests, we reduced the number of different syntaxes you need to know to develop in odoo to 3: