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.
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
feature/WEB-3928: add dependencies to setup.py, update Readme.md #13
feature/WEB-3928: add dependencies to setup.py, update Readme.md #13
Changes from 3 commits
c23813f
d8ac4ab
d744ceb
ebad61c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to be moved to a separate file or add it to the release description https://github.com/Livit/Labster.OAuth2Client/releases ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the release description in github: I'd rather write my docs first as I work with the code, and copy-paste to GH later when releasing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://www.makeareadme.com/
Release History is not that information.
It makes no much sense to load whole release history ( imagine how big it will be in 10+ releases) each time I open repo.
Cookie cutter project suggest having CHANGELOG.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the history then. No one really asked for that. I can live without that. Note to myself: self, stick to the scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. I've mentioned this to several open source projects - nothing worse than having to plough through reams of irrelevant change history before I can even find out what the project is supposed to do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@labstersteve obviously I agree. That is why I intended to put it after the primary content of Readme :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything stopping Python 3.6 from working, or have we simply not tested it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't test it and officially supported versions are python 3.7 and python 2.7. Not my call, just updated a digit in a probably forgotten location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be using
psycopg2-binary
now?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Right now I don't think so, but maybe you can correct me.
The
psycopg2-binary
vspsycopg2
split took place as of version 2.8. Currently LTI requirespsycopg2==2.7.3.1
. If we make the OAuth2Client requirepsycopg-binary
, the installer will install the binary on top of LTI's own requirement (here I may be wrong). And we will end up with LTI using 2.8 de facto. (ugly and unclear situation to end up in) I just did a quick check with pip, and it seems thatpsycopg2
doesn't satisfy the requirement ofpsycopg2-binary
, it got reinstalled on-top. But maybe there is some extra-magic layer for resolving such conflicts and we already use it, I don't know.Apart from that, if we coordinate the change across multiple projects, I don't see any problems.
Quick test I performed (clean venv):
Some reference materials I found useful: