Skip to content

Conversation

@Grimpy101
Copy link

Changes and additions:

  • Update to Python 3.14 (although back-compatibility to Python 3.8 has been tested as well)
    • Moved from comment-based type annotation to Python 3 type hints
    • Changed some parts with unicode type to bytes (Pyscopg2Backend)
    • Removed absolute_import
    • Adjusted tests (list comparison was changed to set comparison, built-in testing dependency is used)
    • setup.py and setup.cfg were removed in favour of pyproject.toml
  • Update dependencies (Flask, psycopg2)
    • Bumped Flask version
    • Switched from deprecated flask._app_ctx_stack to flask.g (PhpBB3)
    • Moved from werkzeug.contrib.cache to cachelib library
    • In tests, set_cookie(...) function arguments were reordered, and server address was specified in client config

Notes:

  • .travis.yml was updated, but the new configuration was not tested
  • All files were structured in accordance with flake8 and mypy configuration

Copy link
Member

@MasterMind2k MasterMind2k left a 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 flake8 to ruff
  • introduce black formatter
  • Introduce uv for 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
Copy link
Member

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?

Copy link
Author

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 ...

Copy link
Member

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
Copy link
Member

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.

Copy link
Author

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.

"""
# Setters for custom fields
custom_fields = self._config.get('CUSTOM_USER_FIELDS', [])
custom_fields: List[Any] = self._config.get('CUSTOM_USER_FIELDS', [])
Copy link
Member

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.

return backend
raise AttributeError('No context available')
backend: Optional[flask_phpbb3.backends.base.BaseBackend] =\
flask.g.get('phpbb3_backend', None)
Copy link
Member

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?

Copy link
Author

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).

Copy link
Member

@MasterMind2k MasterMind2k Dec 28, 2025

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.

class PhpBB3Session(dict, flask.sessions.SessionMixin):
def __init__(self):
# type: () -> None
class PhpBB3Session(Dict[str, Any], flask.sessions.SessionMixin):
Copy link
Member

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?

Copy link
Author

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.

Copy link
Member

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
Copy link
Member

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?

Copy link
Author

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 :)

ACL_OPTIONS_CACHE_TTL: int = 3600 * 1


class BaseBackend:
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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/

Copy link
Author

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 :)

@Grimpy101 Grimpy101 force-pushed the python-update branch 2 times, most recently from 74a0ab1 to 919eebe Compare December 28, 2025 18:23
@Grimpy101
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants