Update dependency to use our fork of odfpy. #343
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- this is to fix a bug in the odfpy library where it can produce non-well-formed XML; - note that in order for this requirement to be satisfied, you will have to update your requirements[-app].txt to include the new dependency; - we are avoiding using "dependency-links" in setup.py, because pip's support for that (i.e. via the `--process-dependency-links` option) is deprecated and has already disappeared once. (See <https://github.com/pypa/pip/pull/1955/files>. https://trello.com/c/EgBoNeW9/145-certain-control-characters-in-suppliers-evidence-corrupts-the-ods-that-buyers-download
|
Okay, have rewritten this to remove the "dependency-links" thing, in favour of an approach that mirrors what we expect users of dm-utils to do, i.e. update |
risicle
approved these changes
Dec 6, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this is to fix a bug in the odfpy library where it can produce non-well-formed XML.
note that in order for this requirement to be satisfied you will have to manually update your requirements files to include the location of the forked library.
https://trello.com/c/EgBoNeW9/145-certain-control-characters-in-suppliers-evidence-corrupts-the-ods-that-buyers-download
** We may not want to land this change **
[We decided to land this change nonetheless.]
Reasons to not land this change:
it's actually a breaking change. When we attempt to bring this version of dm-utils into a frontend, we will find we can't actually install it until the relevant
requirements-app.txtis updated and therequirements.txtis rebuilt.Once you realise you have to update the requirements.txt, there are two options: include the deprecated "--process-dependency-links" flag, or copy in the full git repo details to allow pip to find our new version of odfpy. I think I prefer the latter - adding to the number of warnings doesn't seem ideal.
There's an alternative: we can just change the setup.py here to allow a range (e.g >1.3.6), and then add the repo details into the frontend app(s) requirements.txt files necessary. If we're going to do that anyway, why bother making an incompatible version of dm-utils at all?
ISTM that the alternative approach fits with what seems to pass for current pip 'philosophy', which is that libraries should use setup.py only to declare which versions of their dependencies they are API-compatible with. And then it's (apparently) supposed to be up to people writing applications to pull all of these things together into sensible sets of dependencies (and sources) that are then expressed in a
requirements.txtfile. There's some flaws in that in terms of usability, but that's my understanding - can share references if anyone really wants.