jazzband / django-fsm-log

Automatic logging for Django FSM
https://django-fsm-log.readthedocs.io/en/latest/
MIT License
242 stars 78 forks source link

Fix RemovedInDjango41Warning for default_app_config #136

Closed blueyed closed 2 years ago

blueyed commented 2 years ago

Django 4.0 warns about it already:

django.utils.deprecation.RemovedInDjango41Warning: 'django_fsm_log' defines default_app_config = 'django_fsm_log.apps.DjangoFSMLogAppConfig'. Django now detects this configuration automatically. You can remove default_app_config.

Note that the tests apparently do not trigger it, but projects using it might.

codecov[bot] commented 2 years ago

Codecov Report

Merging #136 (37fbcf3) into master (2da6e3c) will increase coverage by 0.19%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
+ Coverage   95.05%   95.25%   +0.19%     
==========================================
  Files          25       25              
  Lines         506      506              
==========================================
+ Hits          481      482       +1     
+ Misses         25       24       -1     
Impacted Files Coverage Δ
django_fsm_log/__init__.py 100.00% <100.00%> (+33.33%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2da6e3c...37fbcf3. Read the comment docs.

blueyed commented 2 years ago

ref(added in): https://github.com/jazzband/django-fsm-log/commit/fa270cc#diff-b753676ddeaccc240590252172c7f544e33da890fe2aa4b1b855d75bb7b17bf0R3 /cc @MRigal

MRigal commented 2 years ago

@blueyed yes, it is already in there... Therefore I don't know why this PR...

blueyed commented 2 years ago

@blueyed yes, it is already in there... Therefore I don't know why this PR...

Django 4.0 will emit a warning already. I think this PR is fine, but wanted to confirm if there was a specific reason for the lower version you've checked. Maybe you've meant <= also.. anyway, it will prevent an error, but it is good to not emit a warning in the first place - unless I am misunderstanding that Django 4.0 does not handle it properly already, but the warning says it works already.

MRigal commented 2 years ago

Django 3.2 supports the feature, so we check if lower than 3.2, which indeed includes also 4.0. 3.2 does not emit a warning but has the feature (https://docs.djangoproject.com/en/4.0/releases/3.2/#automatic-appconfig-discovery), so I don't know why we should change it.

Maybe you thought we only check against 4.1 or misread something?

blueyed commented 2 years ago

It triggers the warning when using Django 4.0. I have a test suite where those warnings get not only displayed/triggered, but also cause an error (so I really get notified about them). In this case here it was meant to be handled already, but the version check was not fully working as expected. Or do you actually want to trigger the warning with Django 4.0, which I cannot imagine really.

MRigal commented 2 years ago

@blueyed maybe you are not using the actual version in your project.

The code as it is right now, won't trigger the warning with Django 4.0. Your change only impacts the version 3.2, which as I said, accept both variants, without triggering a warning.

blueyed commented 2 years ago

@MRigal Oh, you are correct. I've likely have seen it when the last released version, and then have not checked if that code was included already. And after all your code is more strict. Thanks for you your persistence.. :)