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

session.created property #14

Merged
merged 3 commits into from
Aug 11, 2015
Merged

session.created property #14

merged 3 commits into from
Aug 11, 2015

Conversation

GomZik
Copy link
Contributor

@GomZik GomZik commented Jul 7, 2015

Issue #4: Add created property with timestamp indicating the time when session was created

@asvetlov
Copy link
Member

asvetlov commented Jul 8, 2015

At first I very appreciate your contribution.
But I think we should use a bit another data structure.
Instead of pushing 'created' key into data dictionary we should create dictionary like:

    {'created': created_timestamp,
     'session': session_dict}

where current data moved into session_dict.
The benefits are:

  1. we don't overlap session keys with our auxiliary ones. Say, user may want to have his own session['created'] value, why not?
  2. I suspect created is not the only auxiliary session parameter. Say, we need to control session lifetime based on individual session info, not on system-wide value.

@GomZik
Copy link
Contributor Author

GomZik commented Jul 8, 2015

Thanks for your review. I will rewrite that feature.

@GomZik
Copy link
Contributor Author

GomZik commented Jul 27, 2015

Can you check my PR again, please?

self._created = created

if session_data is not None:
self._mapping.update(session_data)

def __repr__(self):
return '<{} [new:{}, changed:{}] {!r}>'.format(
Copy link
Member

Choose a reason for hiding this comment

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

Please add created info to the repr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, i'l do it

@GomZik
Copy link
Contributor Author

GomZik commented Aug 11, 2015

I made fixes from your comments

asvetlov added a commit that referenced this pull request Aug 11, 2015
session.created property
@asvetlov asvetlov merged commit 70c998b into aio-libs:master Aug 11, 2015
@asvetlov
Copy link
Member

Thanks!

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.

2 participants