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

Update README and pre-commit config #415

Merged
merged 5 commits into from
Dec 1, 2022
Merged

Update README and pre-commit config #415

merged 5 commits into from
Dec 1, 2022

Conversation

timmens
Copy link
Member

@timmens timmens commented Nov 30, 2022

In this PR I added estimagics monthly download numbers as a badge to our README. I further went through our pre-commit hooks and looked for new ones. One new hook that stands out is refurbed. It proposes ways to replace code sections with more pythonic versions; however, it is not ready yet. It is too slow, sometimes proposes wrong things and ignoring arguments does not work well with pre-commit. My recommendation is to use the tool via the console once in a while and adopt some of the proposed changes but not integrate it into our pre-commit hook suite.

Changes

I recommend looking at the .pre-commit-config.yaml changes in split view. I deleted no hooks but added {pyupgrade, name-tests-test, check-toml, forbid-submodules}.

Tasks

@codecov
Copy link

codecov bot commented Nov 30, 2022

Codecov Report

Merging #415 (3b897e2) into main (1e1883a) will increase coverage by 0.00%.
The diff coverage is 96.66%.

@@           Coverage Diff           @@
##             main     #415   +/-   ##
=======================================
  Coverage   92.88%   92.88%           
=======================================
  Files         223      222    -1     
  Lines       17450    17420   -30     
=======================================
- Hits        16208    16181   -27     
+ Misses       1242     1239    -3     
Impacted Files Coverage Δ
src/estimagic/optimization/neldermead.py 90.62% <ø> (-0.10%) ⬇️
src/estimagic/parameters/process_selectors.py 91.12% <ø> (ø)
src/estimagic/optimization/pygmo_optimizers.py 92.66% <66.66%> (+1.44%) ⬆️
src/estimagic/visualization/estimation_table.py 84.93% <85.71%> (+0.25%) ⬆️
src/estimagic/optimization/__init__.py 100.00% <100.00%> (ø)
src/estimagic/optimization/convergence_report.py 100.00% <100.00%> (ø)
src/estimagic/optimization/nag_optimizers.py 89.47% <100.00%> (-0.27%) ⬇️
src/estimagic/optimization/optimize_result.py 95.94% <100.00%> (ø)
...stimagic/optimization/process_multistart_sample.py 53.33% <100.00%> (-2.92%) ⬇️
src/estimagic/optimization/process_results.py 96.15% <100.00%> (-0.28%) ⬇️
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@timmens timmens requested review from janosg and removed request for janosg November 30, 2022 14:19
@timmens timmens changed the title Add downloads badge to README Update README and pre-commit config Nov 30, 2022
@timmens timmens requested a review from janosg November 30, 2022 16:47
@timmens
Copy link
Member Author

timmens commented Nov 30, 2022

@janosg

The new hook name-tests-test fails because

tests/visualization/helpers_test_estimation_table.py does not match pattern "test_.*\.py"

We can either rewrite (this would mean adding the three functions defined in helpers_test_estimation_table.py to the beginning of test_estimation_table.py) or ignore this case. I think I'd prefer a rewrite. What do you think?

@janosg
Copy link
Member

janosg commented Dec 1, 2022

Yes, I prefer the rewrite as well. Can you do it?

Copy link
Member

@janosg janosg left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@timmens timmens merged commit 16a85a4 into main Dec 1, 2022
@timmens timmens deleted the readme branch December 1, 2022 14:31
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.

Use modern pre-commit hooks and implement own
2 participants