search

django-cms / djangocms-picture

django CMS Picture is a plugin for django CMS that allows you to add images on your site.
https://www.django-cms.org
Other
47 stars 61 forks source link

Django 4.2/DjangoCMS 3.11 support #122

Open mogoh opened 1 year ago

mogoh commented 1 year ago

If I ran python manage.py makemigrations after updating to Django 4.2/DjangoCMS 3.11, new migrations will be generated.

Housekeeping

Support of Versions:

py311-django42-cms311
py310-django42-cms311
py310-django32-cms{38, 39, 310}
py39-django32-cms{38, 39, 310}
py38-django32-cms{38, 39, 310}

Testing

Pre commit linting

Code Coverage

Pypi publishing pipeline

Add migrations

There are already two pull requests, for adding migrations.

We should pick one, might need to rebase it onto master and merge it. The other pull request should be closed.

marksweb commented 1 year ago

Would be best with updates based on https://github.com/django-cms/djangocms-audio/pull/32

mogoh commented 1 year ago

I have edited the top comment into a to do list (more or less a list).

mogoh commented 1 year ago

I have opened the pull request #123. It is to fix the tasks of this issue and complement #120.

Question: How do I prepare the environment in a way, that I can run tests/requirements/compile.py? Not every python-version is installed in my package manager.

mogoh commented 1 year ago

I managed to run compile.py with pyenv. I think it should be considered to move a tool like compile.py into a separated tool repository.

mogoh commented 1 year ago

Can someone help me fixing the ci? I tried to configure the .y(a)ml files but they are quit confusing and there is so much tooling involved.

marksweb commented 1 year ago

I think it should be considered to move a tool like compile.py into a separated tool repository.

Because it's a script used to generate the test requirements and not a generic module I'm not sure it'd work as a separate tool. Although with the tweaks that @fsbraun made to it recently it could be made into something that could be packaged up.

fsbraun commented 1 year ago

@marksweb Maybe it's a candidate for a manual github action. Would require a runner with multiple python versions installed and prevent contributors from drowning their hard drives with countless python versions. This might not even be difficult: https://github.com/actions/setup-python/blob/main/docs/advanced-usage.md#specifying-multiple-pythonpypy-version

marksweb commented 1 year ago

@fsbraun yes, love this idea. That'd make it really easy to upgrade requirements as well.

mogoh commented 1 year ago

@marksweb @fsbraun

Although with the tweaks that @fsbraun made to it recently it could be made into something that could be packaged up.

What tweaks? Where is the most recent version? I did some tweaks myself: https://github.com/CERES-RUB/djangocms-picture/blob/feat-django-4-2-support/tests/requirements/compile.py

The problem with the same script in every repository, that they are drifting apart and it is hard to keep track of that. It is for sure possible, that we turn this into a somewhat generic tool / module, that reeds the path of requirements.in from an environment variable or command line parameter. But I understand, if that is not priority right now.

marksweb commented 1 year ago

@mogoh I have tracked down the changes Fabian made to djangocms-alias:

https://github.com/django-cms/djangocms-alias/blob/master/tests/requirements/compile.py

Looks like you were thinking along the same lines with your changes.

If we can develop this into an action, then that solves the issue of the script drifting between repos.

mogoh commented 1 year ago

@marksweb Ah, thanks! I don't know about making it an action, because they are still complicated to me. But if that solves the problem, I am fine with that.

mogoh commented 1 year ago

Here is another problem:

So we are both depending on each other. Proposed solutions:

vasekch commented 1 year ago 
* Integrate one pull request into the other and delete the original pull request. Then merge both.

I'm happy with closing mine PR in favor of #123, mine is one independent file so it will be easy to integrate.

@mogoh feel free to close #120 when done.

mogoh commented 1 year ago

I'm happy with closing mine PR in favor of #123, mine is one independent file so it will be easy to integrate.

@vasekch thx, I will integrate your file. But I am no maintainer, so someone else must close your pull request, once it is done.

mogoh commented 1 year ago

I can't please the coverage ci.

Here it runs with 99%: https://github.com/CERES-RUB/djangocms-picture/actions/runs/5669526899/job/15362500833 But in the individual cases it always has 100%. I don't know why.

https://github.com/CERES-RUB/djangocms-picture/actions/runs/5669526899/job/15362423893 https://github.com/CERES-RUB/djangocms-picture/actions/runs/5669526899/job/15362424081 https://github.com/CERES-RUB/djangocms-picture/actions/runs/5669526899/job/15362424300 https://github.com/CERES-RUB/djangocms-picture/actions/runs/5669526899/job/15362424467

mogoh commented 1 year ago

If I run

tox run -f py38
tox run -f py311
python -m coverage report --fail-under=100

I get a coverage report of 100%.

If I how ever run

tox run -f py311
tox run -f py38
python -m coverage report --fail-under=100

I get a coverage report of 99%.

This suggest to me, that our coverage report is misconfigured and only works for the latest run of tox.

I think the reason, why coverage varies between python version might be the coverage of decorator functions. Also, if coverage vary between python versions, it might be this reason: https://github.com/nedbat/coveragepy/issues/866

I don't know how to achieve 100% coverage and help would be appreciated.

marksweb commented 1 year ago

@mogoh Yeah I've never really liked configuring coverage and often found similar results.

There is the combine command (docs). This combines multiple data files, so the idea being you run this before running the report.

So what happens if you run;

tox run -f py311
tox run -f py38

python -m coverage combine
python -m coverage html --skip-covered --skip-empty
python -m coverage report --fail-under=100
mogoh commented 1 year ago

So, I learned that to combine, one needs to run coverage run --parallel-mode, so that the .coverage files are created like .coverage.<something>. So I did that.

But the problem did not went away. I have no idea why.

Here is what I did:

coverage erase

tox run -f py311
tox run -f py38

coverage combine
coverage html
coverage report --fail-under=100

This leads to 100% coverage

However this does not:

coverage erase

tox run -f py38

coverage combine
coverage html
coverage report --fail-under=100

Only 99% coverage.

mogoh commented 1 year ago

If no one can find a solution, I suggest removing --fail-under=100 and accept less then 100% coverage.

fsbraun commented 1 year ago

Did you look into the different coverage reports. I would assume that some code is only run by py11...?

mogoh commented 1 year ago

Here are some reports:

coverage erase
tox run -f py311
coverage combine
coverage html

yields to

image

coverage erase
tox run -f py38
coverage combine
coverage html

yields to

image

image

(BTW: coverage doesn't generate plain text reports, only short summaries, right? Thus screenshotted html reports.)

fsbraun commented 1 year ago

Hm, that seems more like differences between python versions counting lines..., Since it works with the newer versions, I'd leave the target. We all know that the 99% are ok.

mogoh commented 1 year ago

Sorry, for taking so long to answer.

@fsbraun I am not sure if I understand you correctly.

If we don't change the 100%-code-coverage-target, we are not able to merge the change into master branch, right?

Since it works with the newer versions, I'd leave the target.

What do you mean by that? Leave the target in place (do not change it)? Or leave the target behind and change it?

Because the way I understand you is, that we should change it, but then the rest does not fit. Please clarify.

marksweb commented 1 year ago

@mogoh if the 100% thing is the problem, just remove that from the report command.

mogoh commented 1 year ago

Should this issue stay open, until the new support is released or should we close this? The other pull-requests (#118 and #120) should be closed in any case.