Nan,
Nan,
It is very good, that you wrote also .txt documentation for the
taxprocessor.
I think, it is better to remove long implementation details, even then
the code can change, but the interface remain the same for long time.
For the user is important only to differentiate, that some method is
empty and should be defined, some has a self-explanatory one line code
and other is more complicated.
I created a ticket #1288 [1] for this purpose.
I wrote some comments, example module, not ready. [2]
I moved tax/modules/base/processor.py to tax/modules/processor.py to
be the dependency more evident.
It would be better to start with a minimal safe code with garanteed
compatibility as wrote you and the code which wrote I let under
development and testing so that old modules would not much dependent
on the new code and new modules need not be done by big copy-paste and
simplified interated version of old modules would be accessible as an
option to verify, whether are equivalent to the last penny.
Nan, you forgot to change
self.tax = processor.by_orderitem(self)
The name "by_price_and_product" is longer than I can read by a glance
and with the additional parameter it is much less readable then the
original.
What about "by_price_obj"? (By price and some not trivial object, the
price is usually just the only important and the used keyword
"product=" explains everything.)
[1] https://bitbucket.org/chris1610/satchmo/issue/1288/tax-processors-refactor
[2]
On Mar 19, 4:28 am, Nan <[email protected]> wrote:
> Heh, I'd love to have some documentation to work from, too! I'll
> mostly be duplicating tax.modules.area and working from [1], but will
> jot some notes in the process, and hopefully at least be able to add
> to what exists. I'll be starting in on that some time Monday.
>
> If you'd like documentation for the BaseProcessor in my patch, I'll
> try, but TBH, I'm not entirely clear yet on what each of those methods
> is used for, or where it's called from.
>
> [1]http://groups.google.com/group/satchmo-users/browse_thread/thread/6bb...
>
> On Mar 18, 11:12 pm, Chris Moffitt <[email protected]> wrote:
>
> > To be honest, the patch looks pretty good to me. I haven't thoroughly
> > reviewed it but it appears to be a good implementation.
>
> > Ideally, I'd like some docs on how to implement a custom tax processor so we
> > can get something in place. Other than that I'd like people to review but it
> > looks like a reasonable approach.
>
> > -Chris
>
> > On Fri, Mar 18, 2011 at 9:28 PM, Nan <[email protected]> wrote:
>
> > > I think the best way to explain my patches to Satchmo is to actually
> > > show them to you:
>
> > >https://bitbucket.org/ringemup/satchmo
>
> > > The patch is complete, is very simple, and passes all tests. The
> > > additional processing is minimal; by_product_and_price acts
> > > identically to by_price in the existing processors; and by_price is
> > > used directly for shipping and other situations where there is no
> > > product in question.
>
> > > My next step is to write the custom tax processor; I can use the code
> > > in that to demonstrate to you what it accomplishes and why it needs
> > > the product.
>
> > > On Mar 18, 10:19 pm, hynekcer <[email protected]> wrote:
> > > > OK. How to do it?
>
> > > > I think that more people wants better performance in their situations
> > > > (when you read this forum) and reliability than extending the
> > > > complexity.
>
> > > > The name by_product_and_price sounds redundant, hmm, but it is not.It
> > > > can be useful because price calculation is an expensive operation and
> > > > should not be unnecessarily repeated. It is also useful to keep tax
> > > > modules and discount methods independent and preferably simple. Method
> > > > by_product is not usable for undiscounted prices. Method get_rate
> > > > looks universal and it is basic method for two complicated tax
> > > > processors, but it should be internal method because it is not known
> > > > out of tax processor which context parameters are mandatory for that
> > > > processor.
>
> > > > Why are too much tax.by_* methods? May have been attempts to solve
> > > > rounding problems with 2 decimal places arithmetics with specialized
> > > > methods for one and total. Finally, it was decided at commit 166 three
> > > > years ago to save all 10 decimal places. Then it became less
> > > > important. It was before issues history.
>
> > > > > So patching Satchmo to change those calls shouldn't mean
> > > > > any disruption to people using those processors.
>
> > > > How do you ensure it? How will the new method by_product_and_price
> > > > help you ?
>
> > > > The big problem is with functions which depends on taxer and call only
> > > > by_price:
> > > > shop.models.OrderItem.update_tax
> > > > payment.forms._get_shipping_choices
> > > > product.utils.productvariation_details
> > > > (All 3 templatetags satchmo_discounts.taxed_* can be eventually
> > > > replaced without changing Satchmo or templates.)
>
> > > > Some deep stored methods depends on it.
> > > > shop.models.OrderItem.save
>
> > > > product.modules.configurable.models.Configurable.add_template_context
> > > > payment.forms.SimplePayShipForm.__init__
> > > > Also product itself depends on that. This does look nice at all.
>
> > > > Normal fuction can be dirty patched in a private project by
> > > > manipulation with sys.models['modelname'].__dict__['function_name'],
> > > > but to do it with django.db.models children would be a harakiri.
>
> > > > This is a mystery now
> > > > if self.product.taxable:
> > > > self.unit_tax = processor.by_price(taxclass,
> > > > self.unit_price)
> > > > self.tax = processor.by_orderitem(self)
> > > > After you verify internals and simplify existing tax processors it can
> > > > be probably also simplified without by_orderitem and make it
> > > > deprecated.
>
> > > > These approximately 6 lines with by_price can be replaced by:
> > > > - .... taxer.by_price(product.taxClass, price)
> > > > + if hasattr(taxer, 'need_product_detail'):
> > > > + .... price * taxer.get_rate(product=product)
> > > > + else:
> > > > + .... taxer.by_price(product.taxClass, price)
>
> > > > and you can use it soon.
>
> > > > While it will not make any problems some time (exactly the same
> > > > results, speed, database queries and memory requirements for normal
> > > > taxed projects), it can be something done to be eventually unified in
> > > > BaseProcessor and some redundant methods declared as deprecated and
> > > > not used in a new development.
>
> > > > On Mar 18, 9:52 pm, Nan <[email protected]> wrote:
>
> > > > > To follow up, I think your idea of a base class is a really good one.
> > > > > I'm going to actually try something similar:
>
> > > > > 1) Create a new BaseProcessor and update the existing Tax Processors
> > > > > to inherit from it
>
> > > > > 2) Include a new by_product_and_price method in BaseProcessor, which
> > > > > defaults to simply calling by_price
>
> > > > > 3) In locations where by_price is currently called but product data is
> > > > > available, change the calls to by_product_and_price
>
> > > > > This likely could be accepted into Satchmo trunk without any
> > > > > disruption of existing sites.
>
> > > > > Then I can just write a custom processor that inherits from
> > > > > tax.modules.area and overrides the by_product_and_price method and the
> > > > > _get_location method.
>
> > > --
> > > You received this message because you are subscribed to the Google Groups
> > > "Satchmo users" group.
> > > To post to this group, send email to [email protected].
> > > To unsubscribe from this group, send email to
> > > [email protected].
> > > For more options, visit this group at
> > >http://groups.google.com/group/satchmo-users?hl=en.
--
You received this message because you are subscribed to the Google Groups
"Satchmo users" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/satchmo-users?hl=en.
