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

Added a management command for compatibility checking. #1061

Closed
wants to merge 7 commits into from

Conversation

toastdriven
Copy link
Contributor

Per the discussion on -developers (https://groups.google.com/d/msg/django-developers/0xuznNH7aP0/9AY3w4Ng6SgJ), I started fleshing out an opt-in setup for checking compatibility between Django releases.

Consider this a WIP for now.

@toastdriven
Copy link
Contributor Author

@carljm To answer your previous overall comment:

  1. The only question for me is the scope. Do we make something everyone can use (like 3p apps) or is this just for Django? If it's just Django, I'd gladly cut the discovery code.

  2. IMO, I'd say we prune it to the last 4 (minor) releases (so 1.6/1.7/1.8/1.9 would all stick around until 1.10/2.0). I say this because lots of people find themselves in the situation of skipping a minor release or two of Django & are left to figuring out their compatibility on their own. This should ease that burden if we're committing to it.

@carljm / @jezdez: So, what do you think? Worth continuing down this path or would you prefer a different implementation? I'm not hurt either way, just don't want to push this as the solution if no one is sold on the idea.

@carljm
Copy link
Member

carljm commented May 14, 2013

@toastdriven I'm not sure about making it extensible for 3rd-party apps; it seems like this is pretty reliant on people choosing to run it at the right time, and the obvious time is "when I'm upgrading Django". For it to be useful for third-party apps, the user would have to know which third-party apps are making use of it, and know to run the check command when they upgrade that app. And then we'd be risking having third-party apps that overuse it, or don't prune appropriately, overshadowing important Django upgrade announcements. So I see some risks and not a lot of benefit in making it extensible that way.

And even if we did want that type of extensibility, we could do it by trying an import of a particular module name relative to each installed app, we wouldn't have to do it via filesystem discovery.

@toastdriven
Copy link
Contributor Author

@carljm I altered things to be less extensible. Does that look better to you? I also moved the warning generation up to the management command, so that the utility bits are useful from other code (for instance, you could make a debug-toolbar panel out of the output), hence returning lists of messages.

What do we think about tests on this? I don't mind writing them (should have it IMO) but I'd like to avoid having to continually update tests in the future as the messages would change.

from django.utils import importlib


COMPAT_CHECKS = [
Copy link
Member

Choose a reason for hiding this comment

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

If we're taking this explicit approach (which I think makes sense), why do we still need the indirection of listing strings and dynamically importing from strings? Why not just import the relevant checks modules directly and list them, and not use importlib at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was that it'd be simpler to be able to just add (& remove in future releases) a single line than to do the import & add it to a list to be processed. But you're right, additional complexity & all that. Fixing.

@toastdriven
Copy link
Contributor Author

@carljm I think I've addressed everything you pointed out. Any further thoughts?

if test_runner_setting == new_default:
message = [
u"You have not explicitly set 'TEST_RUNNER'. In Django 1.6,",
u"There is a new test runner ('%s')" % new_default,
Copy link
Member

Choose a reason for hiding this comment

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

s/There/there

@carljm
Copy link
Member

carljm commented May 22, 2013

Other than the stray capital T in the warning (and obviously that it still needs tests, and maybe some docs?), this looks good to me!

@toastdriven
Copy link
Contributor Author

@carljm I've freshly rebased this one, added tests (hope they're alright) & a bit more on docs. IMO, I think this is close to done, but would appreciate one more review.

@carljm
Copy link
Member

carljm commented Jun 14, 2013

Looks good to me!

@toastdriven
Copy link
Contributor Author

Added in SHA: 91f317c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants