Skip to content
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

1.6 #40

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

1.6 #40

wants to merge 11 commits into from

Conversation

pascalfree
Copy link
Contributor

@pascalfree pascalfree commented Feb 13, 2024

TODO

  • check if modifications in juntagrico_custom_sub/downgrade/views_1_5.py are still needed and if yes, apply them to new subscription overview as well.
  • some basimilch specifics have been moved to basimilch.py and shall be moved out of this addon at some point.

@pascalfree pascalfree marked this pull request as ready for review June 30, 2024 23:40
@PaulKC
Copy link
Collaborator

PaulKC commented Dec 27, 2024

Thanks for doing these updates. Is this still relevant or do we need to do more adaptions to make it work with new juntagrico versions?

@pascalfree
Copy link
Contributor Author

It should be complete and work for Juntagrico 1.6.
I would say we can keep in the basimilch.py for now.
Can you maybe test run this version to confirm that it's working? Then we can release it.

[tool.setuptools.dynamic]
version = {attr = "juntagrico_custom_sub.__version__"}

[tool.ruff]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ruff finde ich super. Würde es nicht Sinn ergeben, wenn wir jetzt das setup.cfg löschen würden? Wenn ich es richtig verstehe, wäre in Ruff folgendes das Gleiche:

[tool.ruff.lint.pycodestyle]
max-line-length = 200

https://docs.astral.sh/ruff/settings/#lint_pycodestyle_max-line-length

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codeclimate braucht die setup.cfg noch. Zumindest das letzte mal als ich versucht habe es zu entfernen.

testsettings.py Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hat zwar nicht mit diesem PR direkt zu tun, aber ich die Tests auffürhe, erhalte ich eine Warnung
testsettings.py:66: SyntaxWarning: invalid escape sequence '+'
MIr ist nicht ganz klar, was der Code email.replace("@gmail.com", "(\+\S+)[email protected]") machen soll. Sieht wie Regex aus, aber replace untersützt meines Wissen kein Regex.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ja das kann raus.

requirements.txt Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ich würde noch elegant finden, diese File zu löschen.
Als Ersatz würde ich dann im pyproject.toml dann oberhalb von [tools.setuptools...] das einfügen.
[project.optional-dependencies]
dev = ["coverage", "ruff"]

Im juntagrico-ci.yaml würde ich dann pip install pip install .[dev], damit es ruff und coverage installiert.

Nicht ganz sicher bin ich warum hier die editable flag verwendet wird.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Das würde ich mit dem main repo einheitlich halten und dort addressieren.
Um die Entwicklungs-Abhängigkeiten zu installieren scheint es aber weiterhin üblich mit requirements.txt zu arbeiten. Ich würde erst wechseln, wenn sich ein anderer Ansatz etabliert.

]
readme = "README.md"
license = { file = "LICENSE" }
requires-python = ">=3.8"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gibt es eigentlich jemanden ausser basimilch, der das Plugin braucht? Wenn ja, müssen wir 3.8 noch unterstützen.
Ich wäre eher restriktiv, was dies betrifft. Im Moment würde ich sogar nur 3.12 untersützen, vielleicht noch 3.11.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solange keine Probleme mit älteren Versionen bekannt sind, würde ich keine neuere Version erzwingen. Django 4.2 unterstützt noch python 3.8.

@@ -1,2 +1 @@
name = 'juntagrico-custom-sub'
version = '0.3.0'
__version__ = '0.4.0'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wenn ich es richtig verstehe, gibt es breaking changes, softern jemand die Version <=1.5 von juntagrico verwendet. Darum haben wir die Optionen mit Downgrade.
Im Pyproject-Toml steht aber ohnehin, dass Juntagrico >= 1.6 verlangt wird.
Ich würde diese Downgrade-Optionen weglassen und eine neue Major-Version releasen, sprich 1.0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wenn wir den Sprung machen wollen, würde ich Version 1.6.0 vorschlagen. Damit folgt es dem Schema der anderen add-ons, dass die Version jeweil der kompatiblen Version von Juntagrico gleicht.

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.

3 participants