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

lookup_channel: don't use target_field to get cast function #207

Closed
wants to merge 2 commits into from

Conversation

Hyask
Copy link

@Hyask Hyask commented Aug 2, 2017

At least in Django 1.8.18, target_field does not exists for OneToOneField. Using the field attribute of the relation fixes the exception and makes my forms to work as expected, but I'm not able to test on other Django versions.
Also, I'm not sure if it does not break this #153 again. Could someone check that?

At least in Django 1.8, target_field does not exists for OneToOneField. Using the field attribute of the relation fixes the problem.
@crucialfelix
Copy link
Owner

You can run tox and it will check all combinations of python and django versions. Just install the requirements-test.txt and run make test

@Hyask
Copy link
Author

Hyask commented Aug 2, 2017

Is it not what your CI setup does? Even though, is the specific case of the issue covered?

@crucialfelix
Copy link
Owner

Yes, that's what the CI does.

What I'm saying is that you can easily run it yourself to test before making a PR. You said "I'm not able to test on other Django versions", but it's actually quite trivial to do it. just make test

It did pass the tests, so it probably works. I would have to make sure that part of the code has coverage.

@Hyask
Copy link
Author

Hyask commented Aug 3, 2017

Okay, I see your point. I've added a section in your README to explain that part.

@crucialfelix
Copy link
Owner

I added tests and checked that your fix worked across all versions. Unfortunately I didn't cherry pick your fix, but I did give you credit in the commit message. The fix is now released, Thanks !

@Hyask
Copy link
Author

Hyask commented Sep 10, 2017

That's fine, thanks! :)

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