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

Move to PEP-451 style loader #386

Merged
merged 1 commit into from
Nov 18, 2024
Merged

Conversation

adamchainz
Copy link
Contributor

@adamchainz adamchainz commented May 9, 2024

Fixes #385.

@adamchainz
Copy link
Contributor Author

I didn’t add a changelog note yet because that would conflict with #384.

return None


def wrap_loader(loader, class_name):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A bonus of using this wrapping approach is that it should still work when the settings file is a frozen module, or otherwise provided by some special kind of module. I don’t believe that was supported before, because the old ConfigurationImporter forced using the plain ConfigurationLoader, which had no special logic for loading things.

sys.meta_path.insert(0, importer)
installed = True


class ConfigurationImporter:
class ConfigurationFinder(PathFinder):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also changing this class into a PEP-451 compliant finder, and renaming it as such.

@adamchainz
Copy link
Contributor Author

Tests are passing but codecov is failing, as noted on #384.

@uhurusurfa
Copy link
Contributor

@washeck @pauloxnet Is this application still being maintained? Do you need more hands to manage the PR reviews etc?

@pauloxnet
Copy link
Member

The project is maintained but I'm not using it on a daily basis anymore.
Any help for reviews are welcome. 🤗

@uhurusurfa
Copy link
Contributor

@pauloxnet - ok. Please add me to the project. Also, who is responsible for making releases as it is of no use if PR's are merged but we cannot get new releases done.

@pauloxnet
Copy link
Member

I'm one of the 3 "leads" of the project and I triggered the last release which is aligned with the latest commit. So no risk of PR merged and not released.

I suggest to you to open your PR or (better) review opened PRs you are more interested in, to help it to be ready to be merged (e.g. this PR has some failing checks).

More info about about the project (and how to join it) here: https://jazzband.co/projects/django-configurations

@pauloxnet
Copy link
Member

I opened an issue about Codecov failings #390 (help needed)

@cclauss
Copy link
Contributor

cclauss commented Nov 8, 2024

Please rebase now that tests are passing again.

Copy link

codecov bot commented Nov 8, 2024

Codecov Report

Attention: Patch coverage is 95.52239% with 3 lines in your changes missing coverage. Please review.

Project coverage is 90.75%. Comparing base (f37ed87) to head (b8f66f7).
Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
configurations/importer.py 90.62% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #386      +/-   ##
==========================================
+ Coverage   90.39%   90.75%   +0.35%     
==========================================
  Files          25       27       +2     
  Lines        1197     1211      +14     
  Branches       86       86              
==========================================
+ Hits         1082     1099      +17     
+ Misses         88       86       -2     
+ Partials       27       26       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@pauloxnet
Copy link
Member

Please rebase now that tests are passing again.

@cclauss I tied to solve the conflict, but I can't do anything else here.

If Adam can't push this forward, you are free to open another PR base on this and solve all the conflict and test issue.

@cclauss
Copy link
Contributor

cclauss commented Nov 8, 2024

Just whitespace issues.

checkqa/bin/flake8 .
  ./configurations/importer.py:147:1: W293 blank line contains whitespace
  ./configurations/importer.py:149:13: E303 too many blank lines (2)

@pauloxnet
Copy link
Member

Just whitespace issues.``` checkqa/bin/flake8 . ./configurations/importer.py:147:1: W293 blank line contains whitespace ./configurations/importer.py:149:13: E303 too many blank lines (2)

Can you add code suggestions so I can try to accept those?

configurations/importer.py Outdated Show resolved Hide resolved
@uhurusurfa
Copy link
Contributor

@adamchainz - can you add unit tests to achieve required code coverage?

@adamchainz adamchainz force-pushed the pep_451 branch 2 times, most recently from 2b5bbf6 to 4e5f655 Compare November 18, 2024 12:14
@adamchainz
Copy link
Contributor Author

Rebased.

@adamchainz - can you add unit tests to achieve required code coverage?

Done. The flagged code paths weren't actually tested before, either 😅 .

@uhurusurfa
Copy link
Contributor

Awesome - thanks - looks like we just need the unused imports removed and it will pass.

@uhurusurfa uhurusurfa merged commit 57b6827 into jazzband:master Nov 18, 2024
10 checks passed
@adamchainz adamchainz deleted the pep_451 branch November 20, 2024 16:33
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.

Move ConfigurationLoader to use the PEP-451 style loader
4 participants