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

Feature/callback models bunq/sdk python#40 #43

Merged
merged 25 commits into from
Nov 15, 2017

Conversation

OGKevin
Copy link
Contributor

@OGKevin OGKevin commented Nov 10, 2017

Closes #40
Closes #44

@OGKevin OGKevin added this to the 0.12.3 milestone Nov 10, 2017
@OGKevin OGKevin self-assigned this Nov 10, 2017
Copy link
Contributor

@dnl-blkv dnl-blkv left a comment

Choose a reason for hiding this comment

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

A few comments

:type field: str
:type obj: dict

:return: None|dict
Copy link
Contributor

Choose a reason for hiding this comment

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

dict|None please :D

}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add terminating newline.

}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add terminating newline.

Copy link
Contributor

@dnl-blkv dnl-blkv left a comment

Choose a reason for hiding this comment

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

Was about to approve but then noticed a few more things to comment :D

base_path = os.path.dirname(__file__)
file_path = os.path.abspath(os.path.join(base_path, file_path))

with open(file_path, 'r') as f:
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, just noticed: 'r' is magic! Please make a constant :D

self.assertIsNotNone(expected_model)
self.assertIsNotNone(referenced_model)
self.assertTrue(
self.assertInstanceOfReferencedObject(
Copy link
Contributor

Choose a reason for hiding this comment

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

this should not be called "assert..." but just "is...", because it is not throwing any errors!

class_name
)
or
self.assertInstanceOfReferencedEndpoint(
Copy link
Contributor

Choose a reason for hiding this comment

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

this should not be called "assert..." but just "is...", because it is not throwing any errors!

Copy link
Contributor

Choose a reason for hiding this comment

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

actually these two methods can be combined to one method, such as: isModelReference

Copy link
Contributor

Choose a reason for hiding this comment

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

Then, the content of isModelReference would me something like:

def isModelReference(referenced_model, class_name):
    modelClass = getModelClassOrNone(class_name)

    if modelClass is None:
        return False

    return isinstance(referenced_model, modelClass)

and then

def getModelClassOrNone(class_name):
    if hasattr(object_, class_name):
        return getattr(object_, class_name)
    
    if hasattr(endpoint, class_name):
        return getattr(endpoint, class_name)

    if hasattr(core, class_name):
        return getattr(core, class_name)

    return None

The second method could also be reworked to loop through a constant array of modules and return None if nothing was found in the array.

Copy link
Contributor

Choose a reason for hiding this comment

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

And don't forget to add the docstring with argument and return types please! :D

json_string = f.read()
json_object = json.loads(json_string)
json_string = json.dumps(json_object[
self._KEY_NOTIFICATION_URL_MODEL
Copy link
Contributor

Choose a reason for hiding this comment

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

since we've started... %)
here I'd prefer to have it formatted like this:

json_string = json.dumps(
    json_object[self._KEY_NOTIFICATION_URL_MODEL]
)

Because it is an object element access rather than an array definition, and multi-line approach seems highly odd here.

Copy link
Contributor

@dnl-blkv dnl-blkv left a comment

Choose a reason for hiding this comment

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

Looks good!

@dnl-blkv dnl-blkv merged commit 9412b8d into develop Nov 15, 2017
@dnl-blkv
Copy link
Contributor

@andrederoos

@dnl-blkv dnl-blkv deleted the feature/callback_models_bunq/sdk_python#40 branch November 15, 2017 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants