feature/WEB-3928: add dependencies to setup.py, update Readme.md #13
Conversation
fredmajor
added
documentation
README.md
Outdated
| ------------- | ||
| #### Extras | ||
| As of version `0.2.0` this project provides two extras, `SF` and `providerApp`: | ||
| - `SF` - enables Salesforce JWT grant type support |
There was a problem hiding this comment.
I'm doubt that SF is a good extras name as JWT grant type is a standard and can be used to connect to the other services in the future.
Please avoid using SF and Salesforce.
Could you also add a link to the RFC https://tools.ietf.org/html/rfc7519 ?
There was a problem hiding this comment.
all good points, done
README.md
Outdated
| @@ -142,3 +160,12 @@ folder. | |||
| #### Coverage | |||
| If coverage is installed globally, it can take wrong version inside virtualenv, | |||
| so better uninstall it. | |||
|
|
|||
| ### Release History | |||
There was a problem hiding this comment.
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.
- based on merit: I see no difference between Readme.md vs a separate file. Of course unless we have an established convention of using "a separate file" approach.
- personal opinion: I'd rather have 1 file to remember about doc-wise than >1 files
There was a problem hiding this comment.
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.
According to https://www.makeareadme.com/
A README is a text file that introduces and explains a project. It contains information that is commonly required to understand what the project is about.
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.
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.
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.
@labstersteve obviously I agree. That is why I intended to put it after the primary content of Readme :)
README.md
Outdated
| #### Extras | ||
| As of version `0.2.0` this project provides two extras, `SF` and `providerApp`: | ||
| - `SF` - enables Salesforce JWT grant type support | ||
| - `providerApp` - enables `oauth2_provider.Application` creation by means of |
There was a problem hiding this comment.
Let's rename it to oauth2provider_command or provider_command instead?
| @@ -27,8 +28,25 @@ | |||
| 'Operating System :: OS Independent', | |||
| 'Programming Language :: Python', | |||
| 'Programming Language :: Python :: 2.7', | |||
| 'Programming Language :: Python :: 3.6', | |||
| 'Programming Language :: Python :: 3.7', | |||
There was a problem hiding this comment.
Is there anything stopping Python 3.6 from working, or have we simply not tested it?
There was a problem hiding this comment.
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.
| install_requires=[ | ||
| 'django>=1.11.17,<=1.11.24;python_version=="2.7"', | ||
| 'django>=2.2;python_version>="3.7"', | ||
| 'psycopg2 >= 2.7.3', |
There was a problem hiding this comment.
Should we be using psycopg2-binary now?
There was a problem hiding this comment.
Good point. Right now I don't think so, but maybe you can correct me.
The psycopg2-binary vs psycopg2 split took place as of version 2.8. Currently LTI requires psycopg2==2.7.3.1. If we make the OAuth2Client require psycopg-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 that psycopg2 doesn't satisfy the requirement of psycopg2-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):
╭─[~/PyCharmProjects/venvs]─[fred@pop-os]─[0]─[10056]
╰─[:)] % pip install psycopg2
Processing /home/fred/.cache/pip/wheels/e5/95/7b/9065a4dfe97a9b0e458215ea04fa87c169e20c9bddf49f766a/psycopg2-2.8.4-cp37-cp37m-linux_x86_64.whl
Installing collected packages: psycopg2
Successfully installed psycopg2-2.8.4
╭─[~/PyCharmProjects/venvs]─[fred@pop-os]─[0]─[10057]
╰─[:)] % pip install psycopg2-binary
Collecting psycopg2-binary
Downloading psycopg2_binary-2.8.4-cp37-cp37m-manylinux1_x86_64.whl (2.9 MB)
|████████████████████████████████| 2.9 MB 743 kB/s
Installing collected packages: psycopg2-binary
Successfully installed psycopg2-binary-2.8.4
Some reference materials I found useful:
Scope:
setup.py, to enable complete installation with pip. Dependencies are re-conciliated against LTI and CookieCutter, to ensure no warnings during installationextras, as some are not mandatory for whole projectTesting steps performed:
Jira: https://liv-it.atlassian.net/browse/WEB-3928?atlOrigin=eyJpIjoiNWFjYWE5ZjNmNGVkNDZjM2I1ZTY4MjQyNDk3ZWZlMWMiLCJwIjoiaiJ9