[10.0] product_combination_exclude

[gdgellatly]

Hi,

This is a work in progress. Its another one of those fairly powerful, dense modules which does some cool things but needs some good documentation. I'm just creating a PR for now for feedback particularly on some of the nested loops, and an RFC I'm about to raise but you are welcome to test on runbot.

The premise is that sometimes combination of attributes are not all available and so we want to exclude those on 1, many or all product templates. This allows you to quickly create an exclusion matrix, hand edit if desired and it will prevent those variants from being created, unlink existing ones if it can, or merely deactivate existing ones.

The module is complete aside from above mentioned concerns over cyclomatic complexity - IOW it works, just not sure how efficiently yet.

Still needs docs and tests.

[gdgellatly]

I need some help here. I have a circular xml_id reference. Technically it shouldn't be an issue but it cause travis to fail. I just copied it from the product demo files so maybe it isn't needed but could someone just take a quick look.

The issue is creating attribute lines which require a template, then adding the attribute line to the template in order to create the variants. I'm pretty sure if I don't do it that way I'll need to manually create every variant combination of products.

[lasley]

Hi @gdgellatly - Is this ready for review?

[gdgellatly]

@lasley yes it is now

[lasley]

Thanks @gdgellatly

[lasley]

Remove this line

[lasley]

Can remove this header if there's nothing of note

[lasley]

This is adding a bunch of whitespace. Best to just start and end your strings:

    "summary": "My first line. "
               "and another line"
      

[lasley]

<odoo noupdate="1"> and can remove the data tag

[lasley]

<odoo noupdate="1"> and can remove the data tag

[lasley]

variants_to_deactivate.write(...

[gdgellatly]

as above

[lasley]

Can remove data tag

[lasley]

Any reason that you're not just doing this in CSV? It's much easier and is the standard for the ir.model.access rules - https://github.com/OCA/maintainer-tools/blob/master/template/module/security/ir.model.access.csv

[gdgellatly]

because thats the way mrbob works and I prefer using tools that make things fast to do

[lasley]

No data tag

[lasley]

This should be in CSV

[gdgellatly]

as above

[gdgellatly]

I'm just going to put this in my own repo. Sorry but it just gets too hard. Wait a month or two for a single review and get asked for unneeded commas.

[pedrobaeza]

@gdgellatly Really you retire with some minor comments? OK, go on your own way if you want, but I think you overreacted for some minor suggestions that are interesting and you should listen to for improving your "community" code.

Anyway, if you don't want to do that changes after the rationale for the change, OK, but don't close the PR.

[gdgellatly]

@pedrobaeza I've been down this road before. Probably I've been responsible for others going down it as well and for that I am sorry. OCA does just get too hard nowadays, and if a new PSC is reviewing this hard. Maybe I take your point on some commas, and possibly I overreacted and should just put them in, but still you and I both know where these kinds of reviews end up. I want to stay consistent with upstream for maintainability and porting, PSC wants something OCA perfect. Better to close now and save time.

[pedrobaeza]

@gdgellatly, PSC Dave (and also me) doesn't know about your intention to keep consistency with upstream (it would be interesting to put that on a comment in the method, so we don't need to ask anyway), but he is going to understand for sure and decline in his requests.

Please reconsider your position and open again the PR.

OCA method is not perfect, and it lacks reviewers, but you should also understand that if you don't review any other PRs, people shouldn't review your PR also.

OCA code guidelines and "correctness" is intended to improve your code and avoid any kind of problems derived from the accumulated experience of its contributors, so any suggested improvement will enrich also your code, although it means to work a bit more on your first PRs. Once you get used to these conventions, all your code (OCA published or not), will benefit from them.

My team has hundred of PRs in OCA, and we know that people doesn't review them: https://github.com/pulls?utf8=%E2%9C%93&q=involves%3ATecnativa+is%3Aopen. We are patient, make cross-reviews, fix comments when they are reviewed, and they get finally merged. Meanwhile, we have been using them thanks to our infrastructure (Docker) than can merge PRs directly (buildout can also do it).

[gdgellatly]

@pedrobaeza exactly you get it. You can cross review, know all the rules, have your own trained people etc and still have hundred of outstanding PR's. A single guy like me has no hope.

[pedrobaeza]

I don't think so. It's only a matter of balance of the time spent on OCA. The same as you spend some time reading documentation and so on, and maybe you program a couple of hours a week for this task, you should dedicate a couple of hours a week for OCA related stuff, reviewing, fixing comments, etc. This will enrich you and make you learn a lot, more even than other tasks.

I have now a team, but I have been on my own for 4 years, contributing also with hundred of modules, and I'm what I am now thanks to my involvement in OCA.

[gdgellatly]

@pedrobaeza firstly don't get me wrong, you are a machine and what you do for OCA is amazing. I wish I could have done this in a couple of hours. The design alone was a day or more, and the coding about a week and I spend most of nearly every day writing code. To get it concise, reasonably elegant and testable was quite challenging given upstreams limitations.

However, what seems clear from your position is those that cannot dedicate the same resources or have the same levels of patience should not contribute. I've always maintained a 2:1 review ratio, better where I can.

I don't submit that many PR's, but given we had 40 or so modules from v7 surrounding variants and associated purchase, sale and mrp workflows to port I figured I'd open what I could and review back again where I could. This for me was a large commitment of time. But at this rate and with my deadlines and the style/pace of review that is conducted we are on a timeline of years not months. It just isn't a good fit given the situation, hence closing the open PR's.

So its not about this review in particular, its about the entire time. Telling me I need to invest in better infrastructure just so I can contribute to OCA is not helpful.

[pedrobaeza]

What I talked about a couple of hours was the increment for contributing to OCA, and this tends to be 0 when you get used to the guidelines.

I think that is not fair to say that people can't contribute if they don't dedicate a lot of resources, but the same you learn to code in Python or to work with Odoo, there's a learning to do about collaborative processes, documenting correctly code for others to see it, follow guidelines and so on, and this is the pay needed when contributing to any open-source community (not only OCA).

I'm going to help you with your PRs for not being frustrated with the contribution, but please give us another chance 👼

[lasley]

@gdgellatly - If you are going to take this stance, please close all of your PRs so that I do not continue to waste my time. I just wasted thirty minutes of my time for you to tell me no on things that are simply good practice. The changes I requested would have amounted to all of ten minutes of your's, and would have made your code practices better going forward. If you don't want to get better, that's fine, but please don't waste our time in the process.

[lasley]

PS I'm going to submit a new PR to finish this sometime today, even though I don't need or want this module.

[gdgellatly]

@lasley It was late, I was tired. I over reacted. This is technically complex, indeed in making the requested changes I found a very serious bug, I try very hard to make things match upstream. However it seems clear this is not the place for me. Believe it or not I do know why some of these things are bad, but it is so frustrating to go through 4 linters, wait a month to get human linting. (btw, that isn't directed at you, indeed any review is better than none and I do appreciate your time, and if linting is your preferred review then fine by me, the frustration is the lack of interest in anything variant related from a functional or technical perspective).

[gdgellatly]

Ah I force pushed the changes and can't reopen

[gdgellatly]

all good, I just dropped you a private apology

…

On Wed, May 17, 2017 at 10:06 AM, Dave Lasley ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In product_combination_exclude/models/product_template.py <#255 (comment)>: > + + variants_to_unlink = products.filtered( + lambda p: any([e.attribute_value_ids <= p.attribute_value_ids + for e in exclusion_values])) + + variants_to_deactivate = [] + for variant in variants_to_unlink: + try: + with self._cr.savepoint(), tools.mute_logger( + 'odoo.sql_db'): + variant.unlink() + # We catch all kind of exception to be sure that the operation + # doesn't fail. + except (psycopg2.Error, except_orm): + variants_to_deactivate.append(variant.id) + pass If we don't want to log, that's totally developer preference and was just a recommendation. The reason for this recommendation is for future developers working in this or extended code. A logging.debug hurts nobody, but can help somebody. The blocking comment, and what my (overreacted) question was regarding is the pass. That control statement is superfluous in this context due to the .append above it. pass just helps with our indentation requirement in Python when we don't actually need to do something. Example: Syntax error: try: something() except NameError: print 'test' Pass fixes the syntax error: try: something() except NameError: pass print 'test' — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#255 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE23kMqewA_Pwt0C1kbzbu6c_6bg0oxcks5r6h3dgaJpZM4NClqh> .