Load and Save an ApiContext from and to JSON Data #13

PJUllrich · 2017-08-15T12:14:22Z

I added a small functionality to save and load an ApiContext to and from JSON Data.
Previously, the ApiContext was always saved to and loaded from a file, but this restricted me from saving the config of an ApiContext in a database. I also added unittests for the original ApiContext.save() and ApiContext.restore() functions and their modified versions.

I also added clarification for some of the variables that one need to set in the conf.json in tests/example/.

…o and from JSON data

…nctions in order to test whether modifying them in the last commit broke them or not.

dnl-blkv

A few comments.

dnl-blkv · 2017-08-15T12:23:16Z

bunq/sdk/context.py

@@ -254,33 +254,46 @@ def session_context(self):

        return self._session_context

-    def save(self, path=None):
+    def save(self, path=None, to_json=False):


Functions should do one thing. They should do it well. They should do it only.

-- Uncle Bob

Please make a separate public method (say, to_json) and use it in save.

(also, though not relevant with the change I suggest: Union was introduced in Python 3.5, but we support Python 2 as well, so we do not want it)

dnl-blkv · 2017-08-15T12:25:11Z

bunq/sdk/context.py

        with open(path, self._FILE_MODE_WRITE) as file:
            file.write(converter.class_to_json(self))

    @classmethod
-    def restore(cls, path=None):
+    def restore(cls, path=None, json_data=None):


Same, please create a public classmethod from_json and use it in restore.

dnl-blkv · 2017-08-15T12:26:51Z

bunq/sdk/context.py

        with open(path, cls._FILE_MODE_READ) as file:
            return converter.json_to_class(ApiContext, file.read())

+    def __eq__(self, other):
+        return self.token == other.token \


Please:

return (self.token == other.token and self.api_key == other.api_key and self.environment_type == other.environment_type)

Hmm what about

if not x: return False if not y: return False return True

Looks cleaner IMO.

I am not a big fan of this chaining thing when it comes to just comparison, but since we did it already before, it can be this, yes.

I prefer option 1 with the and statement.

Also, about option 1: You don't seem to PEP8 your code then? At least my IDE gives me a warning when I do that: PEP8: Continuation line under-indented for visual indent

@PJUllrich I might have the wrong indent there; probably adding one tab on the left for each of the indented lines will fix it? :)

But let's go for option 1 then 👍

This seems PEP8 compliant:

return (self.token == other.token and self.api_key == other.api_key and self.environment_type == other.environment_type)

return (self.token == other.token and self.api_key == other.api_key and self.environment_type == other.environment_type)

Should do the trick 😁

OGKevin · 2017-08-15T12:28:57Z

tests/model/generated/test_api_context.py

+
+        self._API_CONTEXT.save(self._TMP_FILE_PATH_FULL)
+
+        with open(self._TMP_FILE_PATH_FULL, self._FILE_MODE_READ) as file:


file is a builtin in python please append a _ at the end of it or rename the variable.

@OGKevin this was here before this PR.

Just wanted to mention that :D
Shall I make it file_?

@dnl-blkv 😮 Yes @PJUllrich please change it to file_

OGKevin · 2017-08-15T12:30:00Z

tests/model/generated/test_api_context.py

+        self._API_CONTEXT.save(self._TMP_FILE_PATH_FULL)
+
+        with open(self._TMP_FILE_PATH_FULL, self._FILE_MODE_READ) as file:
+            context_retrieved = file.read()


Same as above.

@OGKevin this was here before this PR.

OGKevin · 2017-08-15T12:32:33Z

bunq/sdk/context.py

+    def __eq__(self, other):
+        return self.token == other.token \
+               and self.api_key == other.api_key \
+               and self.environment_type == other.environment_type


I don't like the way this looks, maybe an if statement that exits with False if the requirements aren't met and returns True if it never exited?

What do you think @dnl-blkv

We can chain if's, but I am ok with just using some healthy braces

… ApiContext.restore() Added beautiful braces to ApiContext.__eq__() Updated the unittests for ApiContext accordingly

OGKevin · 2017-08-15T13:00:47Z

bunq/sdk/context.py

+        Creates an ApiContext instance from JSON data
+
+        :param data: str
+        :return: ApiContext


Shouldn't this be :rtype:? And could you place a new line before :rtype: &/or :return: please.

OGKevin

Two small comments 😁

OGKevin · 2017-08-15T13:03:21Z

bunq/sdk/context.py

+        """
+        Serializes an ApiInstance to JSON data
+
+        :return: str


Shouldn't this also be :rtype: ?

@OGKevin correct, that should be!

Changed that in the latest commit

OGKevin

All tests passing @dnl-blkv

PJUllrich · 2017-08-15T13:18:38Z

Quick question before you merge this PR:
ApiContext has now a to_json and from_json function, however still contains save() and restore(), which might now be a bit misleading.

I could rename save() to to_file() and restore() to from_file() to make it less ambiguous. What do you think?

dnl-blkv · 2017-08-15T13:20:08Z

bunq/sdk/context.py


-        :rtype: Union[None, str]
+        :rtype: None, str


There is no str anymore :)

Good catch 😁

PJUllrich · 2017-08-15T14:35:16Z

I think this PR can now be merged 😊

dnl-blkv

Please make sure that in all the new code there are newlines after the docstrings (that's the standard we use throughout the SDK) :)

UPD: it is already present everywhere except the places I pointed out.

dnl-blkv · 2017-08-15T14:42:50Z

bunq/sdk/context.py

@@ -254,6 +254,25 @@ def session_context(self):

        return self._session_context

+    def to_json(self):
+        """
+        Serializes an ApiInstance to JSON data


Serializes an ApiContext to JSON string.

dnl-blkv · 2017-08-15T14:43:38Z

bunq/sdk/context.py

+    @classmethod
+    def from_json(cls, data):
+        """
+        Creates an ApiContext instance from JSON data


Creates an ApiContext instance from JSON string.

dnl-blkv · 2017-08-15T14:43:38Z

bunq/sdk/context.py

+
+        :rtype: ApiContext
+        """
+        return converter.json_to_class(ApiContext, data)


newline after the docstring please

dnl-blkv · 2017-08-15T14:43:43Z

bunq/sdk/context.py

+
+        :rtype: str
+        """
+        return converter.class_to_json(self)


Newline after docstring please

OGKevin · 2017-08-15T15:26:04Z

tests/model/generated/test_api_context.py

+from tests.bunq_test import BunqSdkTestCase
+
+
+class ApiContextTest(BunqSdkTestCase):


For consistency, shouldn't this class be named TestApiContext ? @dnl-blkv

rip already merged 🙈

@OGKevin Please create a PR :P

Nice catch!

dnl-blkv · 2017-08-15T15:38:04Z

@andrederoos

…update-sdk-version

… in develop.

Update the SDK version to 1.13.1. (bunq/sdk_python#13)

PJUllrich added 3 commits August 15, 2017 13:06

Added functionality to the ApiContext to save and restore a context t…

67dfb41

…o and from JSON data

Add a clarification of the example config.json to the Test Readme

d4994de

Added unittests for the ApiContext.save() and ApiContext.restore() fu…

f732248

…nctions in order to test whether modifying them in the last commit broke them or not.

dnl-blkv suggested changes Aug 15, 2017

View reviewed changes

OGKevin suggested changes Aug 15, 2017

View reviewed changes

OGKevin assigned PJUllrich Aug 15, 2017

OGKevin added the enhancement label Aug 15, 2017

OGKevin added this to the v.0.10.0 milestone Aug 15, 2017

Extracted a to_json and from_json function from ApiContext.save() and…

eb969c0

… ApiContext.restore() Added beautiful braces to ApiContext.__eq__() Updated the unittests for ApiContext accordingly

OGKevin reviewed Aug 15, 2017

View reviewed changes

OGKevin suggested changes Aug 15, 2017

View reviewed changes

OGKevin reviewed Aug 15, 2017

View reviewed changes

Formatted the docstrings

60a4579

OGKevin previously approved these changes Aug 15, 2017

View reviewed changes

dnl-blkv suggested changes Aug 15, 2017

View reviewed changes

Deleted str from docstring in save()

592b44c

PJUllrich dismissed OGKevin’s stale review via 592b44c August 15, 2017 13:23

dnl-blkv suggested changes Aug 15, 2017

View reviewed changes

PJUllrich and others added 2 commits August 15, 2017 16:49

Making PRs is fun!

60440e0

Little cleanup

7e62a69

dnl-blkv approved these changes Aug 15, 2017

View reviewed changes

dnl-blkv merged commit 35240cd into bunq:develop Aug 15, 2017

OGKevin reviewed Aug 15, 2017

View reviewed changes

OGKevin mentioned this pull request Aug 18, 2017

Break the SDK's dependence on the bunq.conf file #11

Closed

OGKevin added a commit to bunq/tinker_python that referenced this pull request Aug 20, 2018

Update the SDK version to 1.x.x. (bunq/sdk_python#13)

f690154

angelomelonas added a commit to bunq/tinker_python that referenced this pull request Jun 3, 2020

bunq/sdk_python#13 Updated pipfile to latest sdk_python version.

fe7829a

angelomelonas added a commit to bunq/tinker_python that referenced this pull request Jun 3, 2020

Merge remote-tracking branch 'origin/develop' into bunq/sdk_python#13/…

79304e2

…update-sdk-version

angelomelonas added a commit to bunq/tinker_python that referenced this pull request Jun 3, 2020

bunq/sdk_python#13 Small pipfile fix, updated Pipfile.lock and merged…

3f5cf6c

… in develop.

angelomelonas added a commit to bunq/tinker_python that referenced this pull request Jun 3, 2020

Merge pull request #14 from bunq/bunq/sdk_python#13/update-sdk-version

92ea4cd

Update the SDK version to 1.13.1. (bunq/sdk_python#13)


		self._API_CONTEXT.save(self._TMP_FILE_PATH_FULL)

		with open(self._TMP_FILE_PATH_FULL, self._FILE_MODE_READ) as file:

		from tests.bunq_test import BunqSdkTestCase


		class ApiContextTest(BunqSdkTestCase):

Load and Save an ApiContext from and to JSON Data #13

Load and Save an ApiContext from and to JSON Data #13

Conversation

PJUllrich commented Aug 15, 2017 • edited

dnl-blkv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OGKevin Aug 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PJUllrich Aug 15, 2017 • edited

Choose a reason for hiding this comment

OGKevin Aug 15, 2017 • edited

Choose a reason for hiding this comment

OGKevin Aug 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OGKevin Aug 15, 2017 • edited

Choose a reason for hiding this comment

OGKevin left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OGKevin left a comment

Choose a reason for hiding this comment

PJUllrich commented Aug 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PJUllrich commented Aug 15, 2017 • edited

dnl-blkv left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnl-blkv Aug 15, 2017 • edited by OGKevin

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnl-blkv commented Aug 15, 2017

PJUllrich commented Aug 15, 2017 •

edited

OGKevin Aug 15, 2017 •

edited

PJUllrich Aug 15, 2017 •

edited

OGKevin Aug 15, 2017 •

edited

OGKevin Aug 15, 2017 •

edited

OGKevin Aug 15, 2017 •

edited

OGKevin left a comment •

edited

PJUllrich commented Aug 15, 2017 •

edited

dnl-blkv left a comment •

edited

dnl-blkv Aug 15, 2017 •

edited by OGKevin