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

Upgrade for rest-framework 3.X #27

Merged
merged 9 commits into from
Mar 1, 2016
Merged

Upgrade for rest-framework 3.X #27

merged 9 commits into from
Mar 1, 2016

Conversation

estebistec
Copy link
Owner

Here we go. Lots of code gets deleted (always good). The remaining types are so little that perhaps we can convince of their inclusion in rest-framework. CC: @tomchristie

I've kept the tests of list/dict fields to show that the DRF composite types meet the definition that users of this library had relied on.

There is one remaining failing test:

    def test_invalid_item():
        """
        When given an invalid value the ListOrItemField validate method should raise a ValidationError.
        """
        field = ListOrItemField(child=CharField(max_length=5))
        with pytest.raises(ValidationError):
>           field.to_internal_value('123456')
E           Failed: DID NOT RAISE

Whereas the test of a list with items of the wrong length does get the expected validation error. This is the case for any child types that I happened to try. For whatever reason, the validation only kicked in when the child of a ListField. This also gets no validation error:

CharField(max_length=5).to_internal_value('123456')

@kevin-brown
Copy link

You never updated the requirements.txt, so none of the tests were able to run.

@estebistec
Copy link
Owner Author

derp, updated. I was working out of setup.py when I tested the upgrade.

@estebistec
Copy link
Owner Author

Yes, now exactly the one test that I expect fails.

@vperron
Copy link

vperron commented Feb 24, 2016

Any news on this ?

@tomchristie
Copy link

@vperron Some of this is supported in REST framework these days (ListField, DictField)

@vperron
Copy link

vperron commented Feb 24, 2016

Hi @tomchristie , actually I was interested in ListOrItemField for returning data back from ElasticSearch, which transparently handles collections or single objects (https://www.elastic.co/guide/en/elasticsearch/reference/current/array.html)

@vperron
Copy link

vperron commented Feb 24, 2016

I must admit I have no clue how to fix the ValidationError just yet. Did anyone find something about that ?

@vperron
Copy link

vperron commented Feb 24, 2016

Here is the fix I think I'll be using:

    def to_internal_value(self, data):
        if isinstance(data, list):
            return self.list_field.to_internal_value(data)
        # Force field validation. Not necessary on the list_field since DRF calls it recursively.
        self.item_field.run_validation(data)
        return self.item_field.to_internal_value(data)

Does it seem valid enough ?

@estebistec
Copy link
Owner Author

It might be. Let me have a look at the fix tonight.
On Wed, Feb 24, 2016 at 7:08 AM Victor Perron notifications@github.com
wrote:

Here is the fix I think I'll be using:

def to_internal_value(self, data):
    if isinstance(data, list):
        return self.list_field.to_internal_value(data)
    # Force field validation. Not necessary on the list_field since DRF calls it recursively.
    self.item_field.run_validation(data)
    return self.item_field.to_internal_value(data)

Does it seem valid enough ?


Reply to this email directly or view it on GitHub
#27 (comment)
.

@estebistec
Copy link
Owner Author

Thanks @vperron, looks like that does it. I think when I encountered this error it didn't site right with me that this field should be different from others, but it seems the item case is always relying on a container context to trigger validation.

Please test this out. Let me know if it works for you and I'll cut the release for this. If it doesn't, please supply any more test cases we need to consider fixing first.

Thanks again!

@estebistec
Copy link
Owner Author

Hrm... I ran the tests locally on py 3.5.1, travis is failing on 3.5.0. I'll look at this more tomorrow. It's passing on 2.7 / 3.4.

@vperron
Copy link

vperron commented Feb 25, 2016

Hi @estebistec , I had a look and clearly the failure on 3.5.0 is out of my depths. Dunno if that is an issue with Python itself or not. Do DRF tests pass on 3.5.0 ?

@estebistec
Copy link
Owner Author

and by tomorrow I mean "this weekend"... it's been a packed week. Looking today or tomorrow.

so rest framework appears to only be tested through python 3.4. Let's send them a quick pull to add 3.5 and see what happens...

@estebistec
Copy link
Owner Author

nevermind, their travis environments are just named that way. it appears they only test with 3.5.0 :/

@estebistec
Copy link
Owner Author

there we go. found a diff in the pytest version being used. Looked like that fixed it.

Anything else before I merge and release?

@vperron
Copy link

vperron commented Feb 29, 2016

Nope, seems good !

@estebistec
Copy link
Owner Author

Will release tonight (probably not for a few more hours).

On Mon, Feb 29, 2016 at 3:20 AM Victor Perron notifications@github.com
wrote:

Nope, seems good !


Reply to this email directly or view it on GitHub
#27 (comment)
.

estebistec added a commit that referenced this pull request Mar 1, 2016
@estebistec estebistec merged commit ed06a68 into master Mar 1, 2016
@estebistec estebistec deleted the rest-framework-3 branch March 1, 2016 05:39
@estebistec
Copy link
Owner Author

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.

4 participants