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

Add bugbear to pre-commit and some code improvements #694

Merged

Conversation

juanitorduz
Copy link
Contributor

Bugbear (https://docs.astral.sh/ruff/rules/#flake8-bugbear-b) is a great tool for detecting places where bugs can appear. This PR suggests adding this to the code base. On the way I found many places where we had mutable defaults in functions, which are very dangerous https://florimond.dev/en/posts/2018/08/python-mutable-defaults-are-the-source-of-all-evil

from pyfixest.utils.utils import ssc
from pyfixest.utils.utils import ssc as ssc_func
Copy link
Contributor Author

@juanitorduz juanitorduz Nov 3, 2024

Choose a reason for hiding this comment

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

This was a particularly tricky bug as ssc was both a global function and a function parameter.

Copy link

codecov bot commented Nov 3, 2024

Codecov Report

Attention: Patch coverage is 78.37838% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pyfixest/estimation/feols_.py 55.55% 4 Missing ⚠️
pyfixest/utils/dgps.py 0.00% 4 Missing ⚠️
Flag Coverage Δ
core-tests 77.80% <78.37%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
pyfixest/estimation/FixestMulti_.py 80.20% <100.00%> (ø)
pyfixest/estimation/FormulaParser.py 96.48% <ø> (-0.02%) ⬇️
pyfixest/estimation/estimation.py 96.15% <100.00%> (+0.27%) ⬆️
pyfixest/estimation/feiv_.py 98.16% <100.00%> (ø)
pyfixest/estimation/fepois_.py 87.20% <100.00%> (ø)
pyfixest/report/summarize.py 87.00% <100.00%> (+0.21%) ⬆️
pyfixest/estimation/feols_.py 83.24% <55.55%> (+0.25%) ⬆️
pyfixest/utils/dgps.py 6.30% <0.00%> (-0.24%) ⬇️

@s3alfisc
Copy link
Member

s3alfisc commented Nov 3, 2024

Looks great, thank you Juan! I keep on being amazed by the Python tooling that is out there =) Everything looks good based on a first casual glance, though I will take a second look tomorrow before merging (I do not really trust myself after 11:00 😄 ).

@juanitorduz
Copy link
Contributor Author

Sure! Take a detailed review, as some of the changes were made by the pre-commit hook (no rush to merge).

I'll continue adding more guard rails to make the code even better than it is 💪

@s3alfisc
Copy link
Member

s3alfisc commented Nov 4, 2024

Looks all good to me! Looking forward to more guardrails =) Thanks Juan @juanitorduz!

@s3alfisc s3alfisc merged commit c14781e into py-econometrics:master Nov 4, 2024
8 checks passed
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