Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

support polymorphic list with proper type/fields resolution #372

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

Closed
robertobarreda wants to merge 7 commits into django-json-api:develop from robertobarreda:patch-1
Closed

Conversation

Copy link

@robertobarreda robertobarreda commented Sep 14, 2017
edited
Loading

Render polymorphic list of items with different fields/type

Render polymorphic list of items with different fields
Copy link
Collaborator

Hi @robertobarreda. Thanks for the contribution. The project's policy is to only merge work that passes all continuous integration tests. If you're still interested in this branch, could you make sure that CI is passing. I'm happy to review once that check is out of the way. If that's not something you have time for, then we can close out the PR.

Copy link
Collaborator

Ah, it looks like CI might actually be failing because of a factory boy dependency problem. Once #373 is merged, you can probably merge develop into your branch to fix CI.

fix get_polymorphic_serializer_for_instance only for PolymorphicModelSerializer
@robertobarreda robertobarreda changed the title (削除) support polymorphic list (削除ここまで) (追記) support polymorphic list with proper type resolution (追記ここまで) Nov 29, 2017
@robertobarreda robertobarreda changed the title (削除) support polymorphic list with proper type resolution (削除ここまで) (追記) support polymorphic list with proper type/fields resolution (追記ここまで) Nov 29, 2017
Copy link
Collaborator

hi @robertobarreda, it looks like this might be close to ready. I think if you merge the develop branch into this, the branch will get the necessary package update to make the CI build work.

* upstream/develop:
 Set release version to 2.3.0. (#383)
 Allow type field on none polymorphic serializers (#376)
 Fix for resolving source when accessing sub-attr eg b.c (#375)
 Fix typo in usage.md (#379)
 removed duplicate requirement (#377)
 Fix factory boy dependency (#373)
 [WIP] Fix example app (#362)
Copy link
Author

Sorry @mblayman, I merged the develop branch but CI is still failing :(

Copy link
Collaborator

mblayman commented Dec 1, 2017

Looks like there are a couple of issues:

  1. The branch has a flake8 error which needs to be fixed.
  2. There's an import problem. I did a Google search on the exception and found another issues from this repository that's similar. exception on import of the serializer #158 My best guess is that there is some kind of circular import error. Maybe, instead of importing PolymorphicModelSerializer directly, you can import serializers and access the model serializer like serializers.PolymorphicModelSerializer.

Thanks for your diligence in working on this. I'm sure that users who use the polymorphic features will be grateful.

Copy link
Contributor

adsized commented Jan 15, 2018

Just found this pull request and realised that it would probably solve the issue #400 I raised as well as perhaps help with issue #401 which seemed similar.

I can see that some tests still seemed to be failing on some of the travis builds that are configured to use older versions of python. @robertobarreda @mblayman Do you know what was blocking getting this merged in?

Copy link
Collaborator

Travis CI 😉 To keep our sanity on the project, we only merge stuff that passes Travis builds. I suspect there is a problem with circular imports with this PR, but I'm not certain. If someone has time to check out the branch, figure the problem out, and get the branch updated to work in CI, I'd be happy to merge this.

Copy link
Contributor

adsized commented Jan 15, 2018

@mblayman It makes perfect sense only merging stuff that passes the CI builds, I can perfectly understand that 😄

I have checked out out the code from this pull request and had a look at where the errors are coming from. It does seems they were being caused by the serializers import in the renderers file.

From what I have read, the from x import y syntax requires a module to be completely initialized, whereas a simple import does not. By changing this, all of the tests now pass in my environment under both python2.7 and python3.6.

I'd be quite happy to commit my changes. Is there any easy way I can add a commit to this pull request or would I need to open a new one?

Copy link
Collaborator

@adsized, thanks for looking into this. I'd suggest putting up a separate PR. We'll make sure that both you and @robertobarreda get credit for the work and get those issues closed out. Please note that we're now working off of master instead of the old develop branch. I'm not sure if that will cause more work for you or not.

I look forward to seeing the Pull Request! 😄

Copy link
Collaborator

@adsized fixed the import issues in his PR. Thanks @robertobarreda!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /