[11.0][MIG] l10n_nl_tax_invoice_basis #122
Conversation
astirpe
force-pushed
the
11_mig_l10n_nl_tax_invoice_basis
branch
from
31ba563
to
93c1e2a
Compare
| return [ | ||
| ('company_id', '=', domain_params['company_id']), | ||
| '|', | ||
| ] + tax_date_domain + date_domain |
There was a problem hiding this comment.
I like using expression.AND / expression.OR in such cases, but I won't block on this
There was a problem hiding this comment.
@astirpe Should we merge it or do you want to change the line with the remark?
There was a problem hiding this comment.
I would prefer to keep this part of code as it is.
| to_date, | ||
| company_id | ||
| ) | ||
| if not self._context.get('skip_invoice_basis_domain'): |
There was a problem hiding this comment.
shouldn't you check here if your flag on the company is set too?
There was a problem hiding this comment.
and for stopping condition, I always recommend
if $stopping_condition:
return $result
because then you save a level of indentation, and it's clearer what's going on imho
There was a problem hiding this comment.
so #122 (comment) is my only real request for change, the rest take or leave as you prefer
There was a problem hiding this comment.
With latest commit I just return if the stopping_condition is true.
The idea to keep two separate checks, one for the 'skip_invoice_basis_domain' in the context and the other for the country, is that:
if 'skip_invoice_basis_domain' is defined in the context, I skip the next check: the browse of the company and the subsequent check on its country. The intention is to save a prefetch call by the ORM, in case the context already triggered the stop condition.
|
@pedrobaeza Please merge. |
|
Sorry, but this is as localization repository and I shouldn't do that task as general community coordinator, being very country specific and not being able to have an opinion on this. There are several PSC members that can do that if appropriated. |
|
@astirpe can you fix the conflict? then i'll merge |
|
PS: while being at it, please squash your v11 commits |
|
@hbrunn I probably made a mistake: I used the Github interface to resolve the conflicts and now I got a merge with branch 11.0. This causes a lot of unwanted commits in my commits history, see https://github.com/onesteinbv/l10n-netherlands/commits/11_mig_l10n_nl_tax_invoice_basis. Is there a way to revert the merge? |
|
hmmm, I never really used github's edit functions, save for some typos. What I'd do is to squash the commits locally and force push |
astirpe
force-pushed
the
11_mig_l10n_nl_tax_invoice_basis
branch
from
f2ac0b2
to
cd79b89
Compare
|
It seems it's ok now, may I ask you to double check? |
|
LGTM, lets merge it? |
Port of l10n_nl_tax_invoice_basis to v11