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

Added isSessionExpired() method #31. #32

Merged
merged 3 commits into from
Oct 24, 2017
Merged

Conversation

OGKevin
Copy link
Contributor

@OGKevin OGKevin commented Oct 12, 2017

Context

#31

What has been done

This pr introduces a new way to handle SessionContext. Now it is possible to do the following:

if api_context.is_session_expired():
    api_context.reset_session()
    api_context.save()

This way there is no saving an unchanged context.

Closes #31

@OGKevin OGKevin self-assigned this Oct 12, 2017
@OGKevin OGKevin requested a review from dnl-blkv October 12, 2017 20:25
@OGKevin
Copy link
Contributor Author

OGKevin commented Oct 19, 2017

@dnl-blkv all yours 👀

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

# Endpoints not requiring active session for the request to succeed.
_URL_DEVICE_SERVER = 'device-server'
_URI_INSTALLATION = 'installation'
_URI_SESSION_SERVER = 'session-server'
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put these constants after the map (just like we do in PHP).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, you're right! My apologies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

python is not that magical ✨ 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

@OGKevin declare time mixed with run time? Tell me it is not magical :P

@@ -93,7 +103,8 @@ def _request(self, method, uri_relative, request_bytes, params,

uri_relative_with_params = self._append_params_to_uri(uri_relative,
params)
self._api_context.ensure_session_active()
if uri_relative not in self._URIS_NOT_REQUIRING_ACTIVE_SESSION:
self._api_context.ensure_session_active()
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 a newline after the if body! :)


def is_session_active(self):
"""
:return: True if it has expired, otherwise false
Copy link
Contributor

Choose a reason for hiding this comment

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

Please kill this return part: the method is quite obvious!

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 233f052 into develop Oct 24, 2017
@dnl-blkv dnl-blkv deleted the feature/isExpired#31 branch October 24, 2017 12:56
@dnl-blkv
Copy link
Contributor

@andrederoos

@OGKevin OGKevin added this to the 0.12.1 milestone Oct 31, 2017
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