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

Always send command keys in their original order #209

Merged
merged 1 commit into from
Jan 7, 2019
Merged

Always send command keys in their original order #209

merged 1 commit into from
Jan 7, 2019

Conversation

jparise
Copy link
Collaborator

@jparise jparise commented Jan 7, 2019

Some commands are sensitive to the order of their key arguments so
revise the previous code to explicitly build a list of prefixed keys
and a dictionary of remapped keys. This generalization also lets us
remove command-specific checks from this code path.

The list of prefixed keys is used to build the command string
(preserving order).

The dictionary of remapped keys is used to map the prefixed keys from
the response to the key names that were originally provided to the
client.

Copy link
Collaborator

@cgordon cgordon left a comment

Choose a reason for hiding this comment

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

Looks much better this way. I'm a little surprised, in retrospect, that this worked previously, given that the type of the checked_keys value was different depending on which branch of the conditional it took. This approach is much cleaner.

Some commands are sensitive to the order of their key arguments so
revise the previous code to explicitly build a list of prefixed keys
and a dictionary of remapped keys. This generalization also lets us
remove command-specific checks from this code path.

The list of prefixed keys is used to build the command string
(preserving order).

The dictionary of remapped keys is used to map the prefixed keys from
the response to the key names that were originally provided to the
client.
@jparise jparise merged commit c1b21e9 into pinterest:master Jan 7, 2019
@jparise jparise deleted the cmd-keys branch January 7, 2019 23:44
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