Sandy, for me it's OK this way. Regards.
2014-10-20 16:52 GMT+02:00 Sandy Carter <sandy.car...@savoirfairelinux.com>: > Hi, > > My issue with leaving it to subjective criticism is: > > Firstly, if you're new and learning the ropes to contribution to OCA and > you do your homework, you're going to do two things to make sure your > module gets integrated. First you'll check existing modules and emulate > what they do and second you'll read the guidelines. Like I mentioned at > the beginning, there are many cases of X models in files named like > 'product.py', and if you look at the guidelines[1], it is still bare in > many parts. > > We see this today, a new dev will do her work, prepare her module using > these guidelines. When he finally does a pull request, we come down on > him with these subjective reviews, that we are used to, but to the dev, > these seem completely arbitrary. The result is often that the new dev > will redo her module multiple times with no more guidelines than the > review of a select few contributors who have had the time to review her > code. Another possible outcome, is that these PRs get stuck in limbo > where the dev might have moved on to another project. > > A second problem with subjective criticism is when a disagreement is met > or a dev making a PR is unwilling to change his code and chooses to > argue every point. > > Without the points in the review page, a subjective review has very > little weight, you can't just point at the page and say: "These are the > standards we do our review by, this is what everyone who contributes > agrees to for the sake of uniform coding style". Instead we find > ourselves saying: "This is better because it is", "This is what is done > in most modules", "This is not yet on the page". > > These two situations happen quite often. I have multiple less > experienced Odoo developers come to me frustrated because their PRs are > being stalled for what seems to them to be arbitrary reasons. The first > thing they ask is usually "What does she mean by X? This seems > unreasonable." I often reply with "That kind of sucks but I agree with > his review". the second thing they ask is "Where can I find all these > guidelines so I can avoid being stalled on getting my code merged?" And > for that, I would love to be able to refer to the guidelines page[1]. At > the present, I can't really... > > What the consensus I seem to get out of this is: Often it is better to > separate the models (especially in xml), but there are some acceptable > exceptions: > > * Small changes to models > * One to many relationships (single column add to related model) > * At some point, a small change becomes to big and the file must be > split. > * All changes are very closely related to a same feature > * Wizards > * Connectors (I'm adding this one) > > No one seemed to object to naming the Class, .py file and xml files > according to the model (or largest model in the case of filenames). > > Is this something we can agree on? > > [1] http://odoo-community.org/page/website.how-to > > Le 2014-10-20 05:39, Leonardo Pistone a écrit : > > Hi all, > > > > I understand the convenience of finding classes easily, and the > > reasons behind this are sound. Still, I do not sopport such a rule on > > splitting files. > > > > Still, I think we can and should work to improve subjective quality > > and clarity of code. Example: after the machine checked lint, I think > > we can (and should) gradually move to comments like > > > > - this method / this file is long, can you split it? > > - I don't understand what this is for, can you comment or refactor? > > - why do we need that? (comment or refactor) > > - (and one day, gradually...) could you decouple this logic from the > > database schema to a pure class? > > > > TL;DR I oppose a rule on file length and such, but I favor subjective > > criticism to get the code as clear as possible. > > > > Thanks to all for the involvement! > > > > On 10/19/2014 10:46 PM, Graeme Gellatly wrote: > > But for me this comes > >> down to personal preference, not sure it needs to be a convention. > > > > _______________________________________________ > > Mailing list: https://launchpad.net/~openerp-community > > Post to : openerp-community@lists.launchpad.net > > Unsubscribe : https://launchpad.net/~openerp-community > > More help : https://help.launchpad.net/ListHelp > > > > > _______________________________________________ > Mailing list: https://launchpad.net/~openerp-community > Post to : openerp-community@lists.launchpad.net > Unsubscribe : https://launchpad.net/~openerp-community > More help : https://help.launchpad.net/ListHelp > >
_______________________________________________ Mailing list: https://launchpad.net/~openerp-community Post to : openerp-community@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community More help : https://help.launchpad.net/ListHelp
