-
Notifications
You must be signed in to change notification settings - Fork 2
Update dependencies and Python version #20
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
base: master
Are you sure you want to change the base?
Conversation
MasterMind2k
left a comment
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.
Awesome!
A few things to consider:
- Write commits as a story, concentrate to write_reason of change_ into commit message (answer the question why). The answer to question What is answered in change itsef
- Be more graceus with dependencies for packages, limit the lowest allowed version of flask and to next major release (hoping flask follows semver)
And some additional wishes:
- switch from
flake8toruff - introduce
blackformatter - Introduce
uvfor handling dependencies - Go through coerage report and increase test coverage
Thank you for help! Very much appreciated.
requirements.txt
Outdated
| Flask>=0.10 | ||
| typing | ||
| psycopg2>=2.9.11 | ||
| Flask>=3.1.2 |
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.
Dependencies should be more "inclusive" regarding dependencies. Is latestflask really needed?
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.
Sure, we can probably lower the Flask version to 2.3.0 (this is where support for Python 3.7 was dropped and most breaking changes were introduced). For psycopg2, version 2.9.10 would likely suffice (that's where the support for Python 3.7 was dropped as well), although I need to check if it is compatible with Python 3.14 ...
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 can make intermediate version to have 3.8 version supported.
Afaik, only podnapisi.net uses this library :) .
| from __future__ import absolute_import | ||
|
|
||
| import json | ||
| import typing |
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.
Please use module imports.
In python, non-modules (function, classes, etc.) get new reference and are harder to mock. I understad, typing will never be mocked, but it has to be imported in same way as everything else.
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.
Noted 👍
I will correct it.
flask_phpbb3/backends/psycopg2.py
Outdated
| """ | ||
| # Setters for custom fields | ||
| custom_fields = self._config.get('CUSTOM_USER_FIELDS', []) | ||
| custom_fields: List[Any] = self._config.get('CUSTOM_USER_FIELDS', []) |
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 can safely assume this is list of strings.
flask_phpbb3/extension.py
Outdated
| return backend | ||
| raise AttributeError('No context available') | ||
| backend: Optional[flask_phpbb3.backends.base.BaseBackend] =\ | ||
| flask.g.get('phpbb3_backend', None) |
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.
Could you describe why this change?
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.
In Flask 2.3, flask._app_ctx_stack was removed. The preferred way, as I understand it, is to use Flask's g object to store global data (as noted in this GitHub post).
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.
Great explanation! Something that has place in git commit.
flask_phpbb3/sessions.py
Outdated
| class PhpBB3Session(dict, flask.sessions.SessionMixin): | ||
| def __init__(self): | ||
| # type: () -> None | ||
| class PhpBB3Session(Dict[str, Any], flask.sessions.SessionMixin): |
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.
Why inherit from typing.Dict?
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.
Fair, I can change it back, I just wanted more granular typing (dict with string keys in this case). But as far as I know, typing.Dict behaves as dict when inherited.
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.
Very nice!!!
| else | ||
| pip install -r requirements/dev-3.txt | ||
| fi | ||
| pip install -r requirements/dev-3.txt |
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.
Could you also prepare environment management with uv?
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.
I will look into it :)
flask_phpbb3/backends/base.py
Outdated
| ACL_OPTIONS_CACHE_TTL: int = 3600 * 1 | ||
|
|
||
|
|
||
| class BaseBackend: |
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.
That's a big commit change for a python 3.8 compatibility :) . It makes review harder.
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.
Sorry, I'll try to keep these to a minimum and explain them better. If I recall correctly, BaseBackend was only moved below UserAcl so that the type hints in UserAcl could recognize 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.
No worries, you are doing great! Maybe give a Tidy First? book a try.
Also, this is a great oportuity to try out git rebase -i and master "writing commits". Something that helped me was https://cbea.ms/git-commit/
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.
Great, I will have a look at the book! I tried rebasing, so now I have a bit of an idea how to modify commits :)
74a0ab1 to
919eebe
Compare
919eebe to
31818e0
Compare
|
I have tried to address all feedback, and I've added Black formatter and ruff, and introduced uv for package management. Maybe I'll also extend test coverage when I have time. |
Changes and additions:
Notes: