New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Store serializers #4677
Store serializers #4677
Conversation
5d99620
to
9cd39cb
Compare
|
||
|
||
class Command(PootleCommand): | ||
help = "Update database stores from files." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is wrong - from me copying the file
4ad9415
to
197d30d
Compare
Current coverage is 84.11%@@ master #4677 diff @@
==========================================
Files 326 331 +5
Lines 17846 18248 +402
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 14935 15349 +414
+ Misses 2911 2899 -12
Partials 0 0
|
44d5e18
to
59b3aa5
Compare
|
||
def serialize(self): | ||
cache = caches["exports"] | ||
ret = cache.get( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "ret" mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
retinaculum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that this is carried over code (noticed after my first comment), but perhaps we can use a better word.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im gonna leave as is for now, im thinking this i might change this a little soon, but i would rather keep as was for now.
|
||
return ret | ||
def serialize(self): | ||
return StoreSerializer(self).serialize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can be wrong but we use store.serialize()
in import_export
app only. So my question is: why we put StoreSerializer
to pootle_store
app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because its related to Store
serialization specifically, as opposed to eg Project
or TP
serialization etc.
atm, its only hooked up for import_export, but i was going to PR to pretty soon after to use it elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also its for pootle_fs
that is my ultimate use case, but i think that the ability to customize how data is de/serialized is pretty powerful throughout Pootle
for k in kwargs["value"]: | ||
if k not in available_serializers: | ||
return ValidationError( | ||
"Unrecognised pootle.core.deserializers: '%s'" % k) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrecognised pootle.core.serializers
ret = str(store) | ||
cache.set(path, ret, version=rev) | ||
def deserialize(self, data): | ||
self.update(StoreDeserializer(self).deserialize(data)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as we discussed we could deserialize first and then update.
- Store.serialize uses StoreSerialization.serialize - Add deserialize method to Store
lgtm |
@@ -21,6 +21,8 @@ Details of changes | |||
- :djadmin:`refresh_scores` now recalculates user scores and accepts | |||
multiple usernames. | |||
- :djadmin:`contributors` has two new options | |||
- :djadmin:`list_serializers` has been added to view serializers and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expanding in a separate section (and linking to it from here) on what serializers are, what they do and whether users need to care about them or not would be highly needed. Otherwise I feel like anyone reading the release notes or documentation will be pretty lost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #4723.
Fixes #4675
I need to add some tests and docs,
and a cli command so that you can configure the de/serializers for a project.