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

#1 Fixed: Upload VideoField on Admin and Migration of object_id to text_field #2

Closed
wants to merge 6 commits into from

Conversation

iraycd
Copy link

@iraycd iraycd commented Jul 26, 2017

No description provided.

@iraycd
Copy link
Author

iraycd commented Aug 1, 2017

@escaped I have changed few changes which are important.
Instead of object_id being a IntegerField it's now TextField as it should support UUID if that's the primary key.

The test breaks as it's using Django 1.8. I am using Django 1.11

@escaped
Copy link
Owner

escaped commented Aug 8, 2017

@iraycd Thanks a lot for the PR. I really looking forward to it :)
Since you are still adding commits and the tests are failing, i assume that you keep working on it. Please drop me a message if you are ready or need further help. Then i'll do a code review :)

Please make your changes compatible with django>1.7. You can use the provided .tox configuration. Just run the following commands:

pip install tox
tox

It will run the test Suite against Python 2.7/3.5/3.6 and all django version 1.8-1.10. I don't know if the current codebase is compatible with 1.11. But feel free to add that to the configuration.

@escaped
Copy link
Owner

escaped commented Jan 4, 2018

hey @iraycd,
i finally looked into your changes, even though the tests are not working.
Your PR seems to contain two independent features:

  1. Add support for the django admin using a FileField instead of an ImageField (No default_validators #1)
  2. Change the ForeignKey on Format to TextField. (i am not sure, if i like that change. But it seems reasonable).

Could you please split your PR into two seperate PRs. Than i'll have another look.

Thanks a lot :)

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