Skip to content
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

Serialization conversion script #304

Merged

Conversation

lumip
Copy link
Contributor

@lumip lumip commented Jul 12, 2018

Methods convert_stored_pulse_in_storage() and convert_pulses_in_storage() to convert a specified or all pulses from a source storage where the are stored by the old serialization routines to a new destination backend using the new serialization routines.

Closes #296

lumip added 13 commits June 14, 2018 17:29
…format.

Pulse is read using old serialization routines from a source storage backend and destination storage backend to a new using the new routines.
Method should list all available serializables. No tests currently.
convert_pulses_in_storage converts all pulses in a given source StorageBackend
from the old serialization format into the new and stores them in a given
destination StorageBackend.
Both conversion methods now raise an exception if they would need to
overwrite identifiers already existing in the destination backend.
Commit also includes more extensive tests for both conversion methods as
well as some style corretions for already existing tests.
…' into issues/250_serialization_conversion_script
Ensures that the conversion using convert_pulses_in_storage() results in an equal PulseTemplate.
@lumip lumip added this to the New Serialization Routines milestone Jul 12, 2018
@lumip lumip requested a review from terrorfisch July 12, 2018 10:24
@coveralls
Copy link

coveralls commented Jul 12, 2018

Coverage Status

Coverage increased (+0.05%) to 94.046% when pulling 68b2ac5 on issues/250_serialization_conversion_script into 9ef8a0f on issues/250_serialization.

@@ -609,7 +645,7 @@ def deserialize(self, representation: Union[str, Dict[str, Any]]) -> Serializabl
if isinstance(representation, str):
if representation in self.__subpulses:
return self.__subpulses[representation].serializable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please cleaun up to avoid merge conflicts

@@ -141,6 +150,21 @@ def delete(self, identifier):
except FileNotFoundError as fnf:
raise KeyError(identifier) from fnf

def list_contents(self) -> Set[str]:
contents = set()
for dirpath, dirs, files in os.walk(self._root):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os.walk goes through the complete tree. We only return top level jsons in FilesystemBackend.get

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which is why there's a line 159 which aborts the iteration after the first level. The reason to still use os.walk() was that it conveniently separates dirs and files and gives me the base dirpath as well in one single call.

@@ -91,6 +92,14 @@ def delete(self, identifier: str) -> None:
def __delitem__(self, identifier: str) -> None:
self.delete(identifier)

@abstractmethod
def list_contents(self) -> Set[str]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest naming the method keys

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think list_contents() is a more fitting name for what is going on here since the method returns a list of the (available) contents of the storage backend. keys() is a much more generic name and restricts to a plain dictionary "view" of the interface without regarding the context.. I guess it's more "pythonic" though.. ultimately, I don't really care

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think sticking to list_contents is better as we are not returning a view. The return value of a keys method should update if the content of the mapping changes.

@terrorfisch terrorfisch merged commit 5a5b5d7 into issues/250_serialization Jul 12, 2018
@lumip lumip deleted the issues/250_serialization_conversion_script branch July 13, 2018 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants