Skip to content

Add "optionals not required" build #6047

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

Merged
merged 1 commit into from
Jun 22, 2018

Conversation

rpkilby
Copy link
Member

@rpkilby rpkilby commented Jun 21, 2018

Supersedes #6041 - thanks @blueyed!

deps =
django
-rrequirements/requirements-testing.txt

Copy link
Contributor

Choose a reason for hiding this comment

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

While this looks more sane, my approach allows to use -base for everything (i.e. the different Django versions).

Copy link
Member Author

Choose a reason for hiding this comment

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

True, although I'm not sure if it's entirely necessary to have that capability. The basic compat checks are generally going to work, regardless of base Python and Django versions. It should be sufficient just to check once.

In contrast to #6041, testing the optional code was more important, as compatibility between the optional dependencies and the various versions of Python and Django would not be guaranteed.

Copy link
Contributor

@blueyed blueyed left a comment

Choose a reason for hiding this comment

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

I still think that #6041 would be universally better (because you can have -base everywhere), but this is good enough for me!

Thanks for wrapping it up.

@blueyed
Copy link
Contributor

blueyed commented Jun 21, 2018

You should also rename the PR and commit then: "Test base without optionals explicitly" (or similar)

@rpkilby rpkilby changed the title Test optionals optionally Add "optionals not required" build Jun 22, 2018
@carltongibson
Copy link
Collaborator

I always wish GitHub had an asides comment feature... 🙂

I think running with optionals is the way to go. A lot of our users are using Postgres and/or Django Filter in particular and we find issues this way.

Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

OK. Yep. Lets have this. Thanks both!

@carltongibson carltongibson added this to the 3.8.3 Release milestone Jun 22, 2018
@carltongibson carltongibson merged commit 1a17043 into encode:master Jun 22, 2018
@rpkilby rpkilby deleted the optional-optionals branch June 22, 2018 16:43
@blueyed
Copy link
Contributor

blueyed commented Jun 22, 2018

@carltongibson

I always wish GitHub had an asides comment feature...

What do you mean?

@carltongibson
Copy link
Collaborator

Just following along as you and Ryan discuss the issue. I didn’t have a particular comment that was constructive but may have offered an Aside, if such things had a home. 🙂

@rpkilby rpkilby modified the milestones: 3.8.3 Release, 3.9 Release Aug 29, 2018
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants