[logging-too-many-args] Add a regression test for float formatting

[Pierre-Sassoulas]

Type of Changes

	Type
✓	:bug: Bug fix

Description

Closes #9118

[github-actions[bot]]

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit f118b3747366df8b08880ff2acc1c0244650525a

[DanielNoord]

@Pierre-Sassoulas Do you have more WIP here? Or shall we close this?

[Pierre-Sassoulas]

Creating regression tests is work too, why throw it away ? But, I don't have the fix atm no.

[DanielNoord]

Creating regression tests is work too, why throw it away ? But, I don't have the fix atm no.

I think a draft PR with just a bunch of test cases gives a false impression to people stumbling upon the repository that something is being worked upon. We could just post the test cases in the linked issue as a started for somebody who would like to work on this and not have PRs that won't get finished in the PR overview. But perhaps that is just my personal preference. Sorry!

[Pierre-Sassoulas]

Hmm yeah, that make sense if there's only a functional test snipet that can be copy pasted from the issue. This one span multiple files, hard to 'save'. Need take over label ?

[Pierre-Sassoulas]

Or maybe we merge the wrong behavior with proper comment that it's an issue ?

[DanielNoord]

The regression tests are two lines right: https://github.com/pylint-dev/pylint/pull/9121/commits/921be718f01909d7f70f0550cceacf3c5a0c76e2

The rest is just moving files around and duplicating tests into new and old style which probably don't belong there? I'm fine with "needs take over" but I have personally never seen that lead to somebody actually picking up the PR 😅

[Pierre-Sassoulas]

All right, let's merge the refactor and copy paste the functional test snippet then.

[Pierre-Sassoulas]

All right the I was on mobile and didn't see the code. Now that I read it and the context the intent of the MR is due to this : https://github.com/pylint-dev/pylint/issues/9118#issuecomment-1749589514

It was hard to reproduce and the current test is not testing each possibility with each values of logging-format-style=new. Actually the new regression test is 4 lines in 2 files and thus hard to copy paste. I think we should merge and acknowledge that there's a false positive, if someone works on it it's going to make their live easier.

[DanielNoord]

I do think we need to explain why we are mixing old and new style tests in a file that has a docstring of "Tests for logging-too-many-args using old style". That will definitely confuse us later on.

[Pierre-Sassoulas]

It's confusing me right now 😄

[Pierre-Sassoulas]

Updated the docstring for clarity, this is ready for review now.

[Pierre-Sassoulas]

Thanks for the review and thanks for the general cleanup earlier Daniel. I did not check everything but I'm not sure we should loose hope on the "need takes over" label and close them all when they age. Some contributors are giving up really close to completion, there's some value in those MR (but we should close low value or unrecoverable one for sure).

[DanielNoord]

Thanks for the review and thanks for the general cleanup earlier Daniel. I did not check everything but I'm not sure we should loose hope on the "need takes over" label and close them all when they age. Some contributors are giving up really close to completion, there's some value in those MR (but we should close low value or unrecoverable one for sure).

Yeah, those that were close or more recent I kept open. Some of the more controversial ones or low effort ones I closed!

[Pierre-Sassoulas]

Sorry the new comment moved the whole functional test output, had to regenerate 😅

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.82%. Comparing base (4203d87) to head (39038b0). Report is 146 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9121   +/-   ##
=======================================
  Coverage   95.82%   95.82%           
=======================================
  Files         174      174           
  Lines       18810    18811    +1     
=======================================
+ Hits        18024    18025    +1     
  Misses        786      786           

Files with missing lines	Coverage Δ
pylint/checkers/logging.py	94.73% <100.00%> (+0.03%)	:arrow_up:

[github-actions[bot]]

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 39038b0aee7f41fb0b2b35fa7e3b987b23404745