Conversation
azarembok
force-pushed
the
azarembok/django2
branch
2 times, most recently
from
d824919 to
1b1c8ff
Compare
fysheets
left a comment
fysheets
left a comment
There was a problem hiding this comment.
Left questions for clarity and understanding
| @@ -7,5 +7,3 @@ class ConfigurationException(Exception): | |||
| """ | |||
| Exception specific to code annotation configuration problems. | |||
| """ | |||
There was a problem hiding this comment.
Is there any value in this file at all now that it's an empty definition?
There was a problem hiding this comment.
Yes...various places in the code raise ConfigurationException and pass in a specific message. Callers of those functions can check for this exception specifically, which is better than looking for the generic Exception.
| - python: 3.6 | ||
| env: TOXENV=py36 | ||
| env: TOXENV=py36-django111 | ||
| - python: 3.6 |
There was a problem hiding this comment.
Note that we're actually using Python 3.5 for now; we were originally planning on using 3.6, but instead went with the version that comes with our current Ubuntu release. I suspect we'll later be skipping over 3.6 and going straight to 3.8.
There was a problem hiding this comment.
Since we already have 3.6 working, I updated the configuration to run the tests on both 3.5 and 3.6.
| click.echo("\n".join(searcher.errors)) | ||
| # If there are any errors, do not continue | ||
| exit(-1) | ||
| sys.exit(-1) |
There was a problem hiding this comment.
Pre-existing problem, but -1 seems like an odd choice of code given that:
Most systems require it to be in the range 0–127, and produce undefined results otherwise.
There was a problem hiding this comment.
Agreed, I am changing all of these occurrences to use 1 instead.
| # These packages are backports which can only be installed on Python 2.7 | ||
| futures ; python_version == "2.7" | ||
| more-itertools<6 | ||
| pytest==4.6.7 |
There was a problem hiding this comment.
These two should probably get their own comments for why they've been constrained.
| edx-lint==1.4.1 | ||
| filelock==3.0.12 | ||
| funcsigs==1.0.2 | ||
| futures==3.2.0 ; python_version == "2.7" |
There was a problem hiding this comment.
It looks like you ran make upgrade under Python 3; until we officially drop Python 2 support, we need to keep running it under 2.7 to pin all the backports that are only needed under that version.
There was a problem hiding this comment.
I reran it under 2.7.
azarembok
force-pushed
the
azarembok/django2
branch
from
8a978b3 to
72fce65
Compare
3 participants
Description: This PR updates code-annotations to be compatibile with django 2.2. It also adds tox environments for testing django 1.11, 2.0, 2.1, 2.2. Finally it fixes some lint errors that arose while updating dependencies.
JIRA: Link to JIRA ticket
Dependencies: dependencies on other outstanding PRs, issues, etc.
Merge deadline: List merge deadline (if any)
Installation instructions: List any non-trivial installation
instructions.
Testing instructions:
Reviewers:
Merge checklist:
Post merge:
finished.
Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.