Skip to content

Fix #852, anyway this parameter never used #855

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 2 commits into from
Jul 21, 2016

Conversation

valignatev
Copy link
Contributor

Fix for new process api of sqlparse.

@@ -16,7 +16,7 @@
packages=find_packages(exclude=('tests.*', 'tests', 'example')),
install_requires=[
'Django>=1.8',
'sqlparse',
'sqlparse>=0.2.0',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add upper limit as well:

        'sqlparse >=0.2.0,<0.3',

This will help to avoid similar issues with unexpected API changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think sqlparse strictly adheres to semantic versioning and I'm afraid we'll forget to update this when 0.3 gets release, so I prefer the current version.

Copy link
Contributor

Choose a reason for hiding this comment

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

@aaugustin
https://requires.io/ could be used to track dependencies status. They provide a badge, which can be included in README.md, so if some package gets outdated it changes background from green to yellow or red alerting the issue.
They also list all packages with their versions like this: https://requires.io/github/celery/celery/requirements/?branch=5.0-devel
This way, it's easy to mention one requiring update.
There's also an option to be notified about outdated packages by email and/or pull request.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll submit a couple PRs suggesting to add useful badges if you don't mind

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we don't set upper limits, how could our requirements get out of date?

(Also I'm not a fan of cargo-culting badges.)

Copy link
Contributor

Choose a reason for hiding this comment

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

(And I'm especially not a fan of shaming badges that become red if I don't look at the repo for a month or two.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Well it makes sense to consider changing personal preferences given that there's a tool to help handle this. The badge motivates to keep it green, but it's not a must. I believe, automatic notifier does not require adding badge explicitly to work.
Still it's up to you to decide whether to agree with this point.

@valignatev
Copy link
Contributor Author

@webknjaz I'd rather not. Why lock ourselves to old version of other packages? We can also add upper boundary for Django that way. Let's say, <1.10.

@webknjaz
Copy link
Contributor

webknjaz commented Jul 21, 2016

@valentjedi it will still allow to patch version upgrades. It looks they don't follow semver, introducing backwards incompatible changes with minor version upgrade. So only upgrading to patch version change is safe.
On the other hand, there are some services (like requires.io), which help keep track on outdated dependencies.
It's better to maintain dependencies manually, preventing users from having unexpected and unpredictable app failures like this one.

@aaugustin aaugustin merged commit 8e06cb7 into django-commons:master Jul 21, 2016
ryneeverett pushed a commit to ryneeverett/django-debug-toolbar that referenced this pull request Oct 2, 2016
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.

4 participants