Use proxy unit for source_unit when updating from diff #5239
Conversation
phlax
changed the title
| @@ -251,10 +272,14 @@ def diff(self): | |||
| """Return a dictionary of change actions or None if there are no | |||
| changes to be made. | |||
| """ | |||
| diff = {"index": self.get_indexes_to_update(), | |||
There was a problem hiding this comment.
why can't we leave it as it was?
There was a problem hiding this comment.
we can i was using debug_sql on it - thats why it changed
phlax
force-pushed
the
cleanup_proxy_diff
branch
2 times, most recently
from
3ee3dcf
to
6ba1be9
Compare
phlax
changed the title
| unit.save() | ||
| newunit = unit.__class__() | ||
| newunit.source = multistring(["OTHER NEW TARGET", "OTHER NEW TARGETS"]) | ||
| newunit.target = "OTHER NEW TARGET" |
There was a problem hiding this comment.
Does this need some exercising around a multstring based target?
There was a problem hiding this comment.
perhaps - although this is not something ever checked tested - the issue here was that the proxy class didnt have hasplural so would have failed when adding units to a store
|
re queries - this PR increases db calls slightly - due to test changes. If we were testing more extensively with db updating it would have more impact on queries. re timings - this PR seems to improve timings - altho these can be somewhat variable on a local system before: after worth mentioning that this PR improves stability as much as performance - in that its not going back to the disk to check the units (i believe it was doing this but i would need check that 100%) |
| def getnotes(self, origin=None): | ||
| return self.unit["%s_comment" % origin] | ||
|
|
||
| def getcontext(self): |
There was a problem hiding this comment.
@phlax have you considered to put these methods to DiffUnitProxy class?
There was a problem hiding this comment.
i think they are useful for any unitproxy - no?
There was a problem hiding this comment.
I used UnitProxy as base class but I didn't use any of its methods here
a56c885
There was a problem hiding this comment.
fair point - im wondering if its useful outside of diff - here its used in the update too - and what we should call it
phlax
force-pushed
the
cleanup_proxy_diff
branch
from
6ba1be9
to
5f9d2e2
Compare
|
Commit "Add getcontext, hasplurals, isfuzzy, isobsolete and getid to UnitProxy" message doesn't match its changes. |
| or getattr(self.source, "plural", None))) | ||
|
|
||
| def getnotes(self, origin=None): | ||
| return self.unit["%s_comment" % origin] |
There was a problem hiding this comment.
How does this render in case no origin is specified? Perhaps it is better not to default to None?
There was a problem hiding this comment.
hm - i think it should return ""
|
oops - need to move that method - and kill commit... |
phlax
force-pushed
the
cleanup_proxy_diff
branch
2 times, most recently
from
05d37c9
to
2d71270
Compare
|
lgtm |
| source_unit = self.source_store.findid(uid) | ||
| if source_unit and source_unit.getid() not in self.target_units: | ||
| source_unit = self.source_units.get(uid) | ||
| if source_unit and uid not in self.target_units: |
There was a problem hiding this comment.
uid == source_unit.getid() ?
phlax
force-pushed
the
cleanup_proxy_diff
branch
from
2d71270
to
7f9abb2
Compare
|
lgtm |
When StoreDiff creates its diff of changes the code then retrieves the source units individually (either from the file or the db) - although it already has the information required to update.
This PR uses a proxy class to update the unit without going back to db/fs
this needs tests to ensure that a plural from a source db unit is updated correctly
UPDATE: added some tests to ensure that plural form units are added correctly when copied from db