Skip to content

use dict instead of if/else logic #2093

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.

Sign up for GitHub

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

Jump to bottom
Merged
untitaker merged 1 commit into from
Dec 21, 2016
Merged

use dict instead of if/else logic #2093

untitaker merged 1 commit into from
Dec 21, 2016

Conversation

@r0fls
Copy link
Contributor

@r0fls r0fls commented Nov 20, 2016

No description provided.

RonnyPfannschmidt
RonnyPfannschmidt suggested changes Nov 21, 2016
View reviewed changes
flask/sessions.py
def dumps(self, value):
return json.dumps(_tag(value), separators=(',', ':'))

LOADS_MAP = {

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

RonnyPfannschmidt
RonnyPfannschmidt reviewed Nov 21, 2016
View reviewed changes
flask/sessions.py Outdated
' u': uuid.UUID,
' b': b64decode,
' m': Markup,
' d': parse_date

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ThiefMaster
ThiefMaster reviewed Nov 21, 2016
View reviewed changes
flask/sessions.py Outdated
' b': b64decode,
' m': Markup,
' d': parse_date,
}

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@davidism
Copy link
Member

davidism commented Nov 21, 2016

Haven't looked at it in a while, but make sure this is in line with #1452.

tuukkamustonen
tuukkamustonen reviewed Nov 21, 2016
View reviewed changes
flask/sessions.py Outdated
elif the_key == ' d':
return parse_date(the_value)
# Check the key for a corresponding function
return_function = self.LOADS_MAP.get(the_key, None)

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@r0fls
Copy link
Contributor Author

r0fls commented Nov 22, 2016

This does appear to be made obsolete by #1452, thanks @davidism. However that PR has been open for a year and a half, so this simple change might make sense while we're waiting for that to be approved. I'll leave that decision to the discretion of those with merge rights though.

@RonnyPfannschmidt
Copy link
Contributor

RonnyPfannschmidt commented Nov 22, 2016

@davidism at first glance #1452 looks like a over-generalization with quite a performance penalty :/

@RonnyPfannschmidt RonnyPfannschmidt mentioned this pull request Nov 22, 2016
Closed
@untitaker untitaker merged commit 0e79aba into pallets:master Dec 21, 2016
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ThiefMaster ThiefMaster ThiefMaster left review comments

+2 more reviewers

@tuukkamustonen tuukkamustonen tuukkamustonen left review comments

@RonnyPfannschmidt RonnyPfannschmidt RonnyPfannschmidt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@r0fls @davidism @RonnyPfannschmidt @tuukkamustonen @ThiefMaster @untitaker