Code review comment for lp:~npg-team/openobject-addons/shipping_api_ups_npg

Hello,

I would like to explain a little bit more why this merge proposal was rejected, in 2 parts: our policy for merge proposals, then some specific hints.

1. Merge Proposal Acceptance Policy
===================================
There may have been contradicting messages about how and when it is useful to make a merge proposal.
We would like to state this policy very clearly, especially now that extra-addons have been deprecated due to the introduction of OpenERP Apps. So we have now added an official Merge Proposal Acceptance Policy to our contributor documentation, please have a look: http://bit.ly/openerp-contrib-mp

2. Remarks specific to this merge proposal
==========================================
In approximate order of importance:

- based on your minimal description (it would help to have more), it looks like this module is a general purpose extra feature that could be published on OpenERP Apps as explained in the policy, but does not need to be included in official addons now
- the size of the merge proposal diff is extremely large (+8000 lines) compared to the scope:
  + the UPS XML generation code looks duplicated in *MANY* places when it should be much smaller, simpler and modular
  + the XML generation code, on top of being duplicated and too verbose, is mixed inside the business code. It should be part of an abstraction in charge of contacting UPS, and that lies outside of the OpenERP business code: less code, cleaner, more modular and extensible
  + once again, you included a "tiny_sxw2rml.py" script and dependent files that look totally useless and out of place (1100+ lines!)
- lots of duplicated code from shipping_api module, e.g. package_types, etc..
- incorrect use of email template (should not need to evaluate the template yourself)
- outdated mail/email_template API - 6.1 has a new, much cleaner email API, please look at it - your module will not work on 6.1 as it is
- you are overriding methods from original addons without calling super(), and thus duplicating more code
- if this module is related to shipping, why does it add a payment_method on sale.order?
- instead of minidom, OpenERP official addons should use lxml/etree for XML parsing/generation
- adding shipping related field on email.template looks like bad design: breaking encapsulation
- horrible python code for modifying mako templates
- bad python style - see coding guidelines (type comparisons -> isinstance(), dict usage, etc.)
- no proper description of the features in the module manifest, nor how to use them
- a LOT of remaining commented out code lines (thousands of lines!)
- many useless python imports

I hope this helps,

Thanks!