Closed AsadpourMohammad closed 1 year ago
The init file issue was fixed. I don't know if my way of handling it was in check with the PEP guidelines or not. But the checks all passed, so I think it's correct.
Hi. Thanks for the PR! It generally looks fine, though one problem that I see is that it doesn't separater the requirements.txt
file. One way to solve this would be to split that into requirements_mongodb.txt
and requirements_sqalchemy.txt
and add a note to the readme which to install when. Additionally, requirements.txt
should then just list -r requirements_mongodb.txt
and -r requirements_sqalchemy.txt
. What do you think of that?
Does the requirements.txt file install both dependencies for mangodb and sqlalchemy when installing the ptbcontrib package? Because when I was using the ptbcontrib package, I had installed SQLAlchemy myself before that, and only was forced to install pymongo after I got an error of the package being mentioned as a dependency of PTBMongoDBJobStore in the init file. It wasn't installed by pip. Though still your suggestion might be good because even though it might not effect the user experience in installation, it's good for separation of concerns and might help the dev experience in the future.
I think we could also have requirements.txt file in each subpackage of ptb_jobstores. Would that work too?
Yes, where exactly the separate requiremts files are located doesn't really matter as long as the relative link within requirements.txt
is correct.
There is one more problem I hadn't thought about so far though: When installing via $ pip install "ptbcontrib[extension1,extension2] @ git+https://github.com/python-telegram-bot/ptbcontrib.git@main"
, the setup.py
script will install the dependencies listed in requirements.txt
, which would be both. If we want to allow to install only parts of the dependencies, we'd either have to again split the job stores up into ptb_{mongodb, sqlalchemy}_jobstore_
or add some extra logic to setup.py
. In the latter case one could make it so that "ptbcontrib[contribname_postfix] …
looks for a directory starting with contribname
and checks if there is a file called requirements_postfix.txt
in there.
I'm not quite sure which option I like better …
If more jobsotres are added in the future, would separation of packages lead to duplicate code? Though I guess there are ways to avoid duplication of logic. I think the second option might be better because if another jobstore to is going to be added in the future, setup.py
is ready to accept.
And I hope that these changes are going to be made by the maintainers because I don't know what would be the best way to separate the requirements.txt
files or how to change the setup.py
file.
If more jobsotres are added in the future, would separation of packages lead to duplicate code?
Not necessarily. the adapter class should cover most of that.
And I hope that these changes are going to be made by the maintainers because I don't know what would be the best way to separate the
requirements.txt
files or how to change thesetup.py
file.
I'll look into it
I took the liberty to push to your branch and also used the opportunity to use the pathlib
library in setup.py
. Please update the requirements files and add a note to the readme about how to install via the extra dependencies :)
So... the tests failed. Should I not have deleted the requirements.txt file after including the dependencies inside the two other requirement files?
Yes, please re-add the requirements file
Additionally, requirements.txt should then just list -r requirements_mongodb.txt and -r requirements_sqalchemy.txt
error in ptbcontrib setup command: 'extras_require' must be a dictionary whose values are strings or lists of strings containing valid project/version requirement specifiers.
Is this error related to any change I made?
Thank you very much for the contribution :)
Separated SQLAlchemy and MongoDB jobstores into different packages to avoid unnecessary installation of both dependencies.
(I realized after commiting that I forgot to clear init.py file of ptb_jobstores package. This needs to be fixed.)