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.

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

Handle querystring parameters as JSON encoded values. Fixes #1217 #1252

Merged
merged 2 commits into from
Jun 5, 2017
Merged

Handle querystring parameters as JSON encoded values. Fixes #1217 #1252

merged 2 commits into from
Jun 5, 2017

Conversation

Natim
Copy link
Member

@Natim Natim commented Jun 5, 2017 •
edited
Loading

Fixes #1217

  • Add tests.
  • Add a changelog entry.

@Natim Natim requested a review from magopian June 5, 2017 09:16
magopian
magopian reviewed Jun 5, 2017
View reviewed changes
Copy link
Contributor

@magopian magopian left a comment

Choose a reason for hiding this comment

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

LGTM appart the two last test modifications that I don't understand. I'll test this straight away as you deployed it on kinto-ota, thanks!

tests/core/test_utils.py
@@ -44,12 +59,12 @@ def test_float(self):
self.assertEqual(native_value('3.14'), 3.14)

def test_true_values(self):
true_strings = ['True', 'on', 'true', 'yes']
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because they are not valid JSON values and we don't use it, so I'd like to change the behavior to stick with the JSON spec there.

magopian
magopian requested changes Jun 5, 2017
View reviewed changes
Copy link
Contributor

@magopian magopian left a comment

Choose a reason for hiding this comment

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

I've tested it, it doesn't seem to be fixing the issue :/

@Natim
Copy link
Member Author

Natim commented Jun 5, 2017

It doesn't fix #1217 but it is a first step toward. I think the Postgresql issue will be handled by #1220

magopian
magopian approved these changes Jun 5, 2017
View reviewed changes
Copy link
Contributor

@magopian magopian left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Ok so I'll approve it

@Natim Natim merged commit 05fe70f into master Jun 5, 2017
@Natim Natim deleted the 1217-handle-querystring-parameters-as-json-encoded-values branch June 5, 2017 12:21
@glasserc
Copy link
Contributor

glasserc commented Jun 5, 2017

I believe this fixes #1216 and #1215.

@Natim
Copy link
Member Author

Natim commented Jun 5, 2017

I am not sure we should add specific tests to make sure it works I guess.

leplatrem
leplatrem reviewed Jun 6, 2017
View reviewed changes
CHANGELOG.rst
**Bug fixes**

- Handle querystring parameters as JSON encoded values
to avoid treating number as number where they should be strings. (#1217)
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be an API breaking change (although off and no were not documented...)

@leplatrem leplatrem mentioned this pull request Jun 8, 2017
Merged
4 tasks
This was referenced Jun 8, 2017
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leplatrem leplatrem leplatrem left review comments

@magopian magopian magopian approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Natim @glasserc @magopian @leplatrem