openerp-india team mailing list archive

["Alexandre Fayolle @ camptocamp" <alexandre.fayolle@xxxxxxxxxxxxxx>]

On 22/05/2012 07:45, Amit Parik (OpenERP) wrote:
> Hello Alexandre,
>
> Yes, you are right we have passed the product_uom parameter on _bom_find
> method but we didn't used this argument under this function.
>
> But what ravish is said, we have called this method many times under
> some function at there we have used this argument. Just check line#565
> on mrp.py under  product_id_change we have called  _bom_find method at
> there sticky needed this argument also it need on some other method . So
> we can not remove this argument.
>
> IF we have to remove this parameter then we must update all place at
> where this function is called, I don't think it's good.
>
> So this is not a bug and I am closing this issue.
>
> Thanks for understanding!
>

Hello Amit,

I'm saddened that you chose to close this but as invalid, because I feel
that there really is an issue here.

I can understand that changing the prototype of the function poses some
backward compatibility issues (and this is why I don't propose changing
this in the stable branch).

Denying the existance of the problem is not the appropriate way of
dealing with it. What I propose is:

* the argument can be deprecated first (and it is even possible to issue
a DeprecationWarning each time the function is called with product_uom
argument set to a non None value)
* the docstring must make it clear that the argument is deprecated and
ignored
* a quick grep in the addons source shows a total of 7 calls to
_bom_find in addons and addons-community which is not unmanageable by my
standards, meaning it is easy to fix the most common uses of that
method, leaving other module authors the responsibility of dealing with
the DeprecationWarning.

If we could come to an agreement that this is the way to go, I can
propose a patch against trunk doing the above.

The reason I have not done this so far is that I'm not sure that the
real bug is not that the argument is not used in _bom_find, and that the
proper fix is to change the implementation to filter out BoM which don't
have the correct product_uom, and I can probably come up with a patch
for this too if there is consensus that this is the correct thing to do
(but this patch can result in some code no longer working as intended
since the results returned by _bom_change will not be the same as
before, e.g. if no BoM is found because of the product_uom argument).

In both cases, closing the bug as Invalid is not appropriate.

Thanks for your time and dedication to OpenERP.

-- 
Alexandre Fayolle
Chef de Projet
Tel : + 33 (0)4 79 26 57 92

Camptocamp France SAS
Savoie Technolac, BP 352
73377 Le Bourget du Lac Cedex
http://www.camptocamp.com


** Changed in: openobject-addons
       Status: Invalid => New

-- 
You received this bug notification because you are a member of OpenERP
Indian Team, which is subscribed to OpenERP Addons.
https://bugs.launchpad.net/bugs/999103

Title:
  [trunk] mrp_bom._bom_find: unused argument product_uom

Status in OpenERP Addons (modules):
  New

Bug description:
  The signature of mrp_bom._bom_find has a product_uom parameter which
  is not used. I think it should be deprecated and removed altogether.
  At the very least the docstring should be fixed to say that the
  argument is ignored, but I believe some code could be simplified and
  made clearer by removing that argument recursively accross the call
  stack.

To manage notifications about this bug go to:
https://bugs.launchpad.net/openobject-addons/+bug/999103/+subscriptions