[PATCH 2/2] u_boot_pylib: Clean up pylint warnings in gitutil.py

[Tom Rini]

On Sat, Mar 15, 2025 at 03:50:55PM +0000, Simon Glass wrote:
> Hi Tom,
> 
> On Sat, 15 Mar 2025 at 15:23, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Sat, Mar 15, 2025 at 02:48:18PM +0000, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Fri, 14 Mar 2025 at 15:36, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Fri, Mar 14, 2025 at 02:45:27PM +0000, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Mon, 17 Feb 2025 at 19:23, Tom Rini <trini at konsulko.com> wrote:
> > > > > >
> > > > > > On Sun, Feb 09, 2025 at 02:26:01PM -0700, Simon Glass wrote:
> > > > > >
> > > > > > > Reduce the number of warnings in this file.
> > > > > > >
> > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > > ---
> > > > > > >
> > > > > > >  tools/buildman/func_test.py   |   4 +-
> > > > > > >  tools/u_boot_pylib/gitutil.py | 240 ++++++++++++++++++----------------
> > > > > > >  2 files changed, 128 insertions(+), 116 deletions(-)
> > > > > >
> > > > > > As I think these all were warnings from whatever other linting you had
> > > > > > mentioned in another thread, I deferred this patch as it also doesn't
> > > > > > cleanly apply.
> > > > >
> > > > > Some of it, yes. But most of it is fixing things that we should fix.
> > > >
> > > > To be clear, to the extent any of it was pylint problems, they've now
> > > > been addressed as we've moved to pylint 3.3.4.
> > >
> > > Actually, I don't know about those problems, but that's fine.
> > >
> > > >
> > > > > This is going to open up a significant diff for patman, unfortunately,
> > > > > just as I am adding new features for series management. So please
> > > > > consider applying it. I can rebase if you like?
> > > >
> > > > Yes, that's fine, please just explain what's showing the warnings or
> > > > otherwise update the commit message.
> > >
> > > Before:
> > >
> > > ************* Module u_boot_pylib.gitutil
> > > tools/u_boot_pylib/gitutil.py:1:0: C0114: Missing module docstring
> > > (missing-module-docstring)
> > > tools/u_boot_pylib/gitutil.py:13:0: C0103: Constant name
> > > "use_no_decorate" doesn't conform to UPPER_CASE naming style
> > > (invalid-name)
> > > tools/u_boot_pylib/gitutil.py:40:19: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:65:20: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:72:25: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:64:12: W0612: Unused variable 'msg'
> > > (unused-variable)
> > > tools/u_boot_pylib/gitutil.py:114:21: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:121:25: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:122:17: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:139:41: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:141:40: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:146:4: R1705: Unnecessary "elif" after
> > > "return", remove the leading "el" from "elif" (no-else-return)
> > > tools/u_boot_pylib/gitutil.py:151:15: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:153:25: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:171:11: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:189:21: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:243:22: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:255:22: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:272:22: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:305:22: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:317:22: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:320:0: R0913: Too many arguments (6/5)
> > > (too-many-arguments)
> > > tools/u_boot_pylib/gitutil.py:320:0: R0917: Too many positional
> > > arguments (6/5) (too-many-positional-arguments)
> > > tools/u_boot_pylib/gitutil.py:345:16: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:347:12: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:353:4: R1705: Unnecessary "else" after
> > > "return", remove the "else" and de-indent the code inside it
> > > (no-else-return)
> > > tools/u_boot_pylib/gitutil.py:402:16: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:416:7: R1714: Consider merging these
> > > comparisons with 'in' by using 'suppresscc in ('all', 'cccmd')'. Use a
> > > set instead if elements are hashable. (consider-using-in)
> > > tools/u_boot_pylib/gitutil.py:420:15: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:433:0: R0913: Too many arguments (12/5)
> > > (too-many-arguments)
> > > tools/u_boot_pylib/gitutil.py:433:0: R0917: Too many positional
> > > arguments (12/5) (too-many-positional-arguments)
> > > tools/u_boot_pylib/gitutil.py:433:0: R0914: Too many local variables
> > > (17/15) (too-many-locals)
> > > tools/u_boot_pylib/gitutil.py:512:19: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:514:19: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:520:24: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:433:0: R1710: Either all return
> > > statements in a function should return an expression, or none of them
> > > should. (inconsistent-return-statements)
> > > tools/u_boot_pylib/gitutil.py:435:36: W0613: Unused argument
> > > 'get_maintainer_script' (unused-argument)
> > > tools/u_boot_pylib/gitutil.py:592:14: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:593:8: R1720: Unnecessary "else" after
> > > "raise", remove the "else" and de-indent the code inside it
> > > (no-else-raise)
> > > tools/u_boot_pylib/gitutil.py:601:18: C0209: Formatting a regular
> > > string which could be an f-string (consider-using-f-string)
> > > tools/u_boot_pylib/gitutil.py:682:4: W0603: Using the global statement
> > > (global-statement)
> > >
> > > ------------------------------------------------------------------
> > > Your code has been rated at 8.41/10 (previous run: 8.37/10, +0.04)
> > >
> > >
> > > After:
> > >
> > > ************* Module u_boot_pylib.gitutil
> > > tools/u_boot_pylib/gitutil.py:1:0: C0114: Missing module docstring
> > > (missing-module-docstring)
> > > tools/u_boot_pylib/gitutil.py:325:0: R0913: Too many arguments (6/5)
> > > (too-many-arguments)
> > > tools/u_boot_pylib/gitutil.py:325:0: R0917: Too many positional
> > > arguments (6/5) (too-many-positional-arguments)
> > > tools/u_boot_pylib/gitutil.py:439:0: R0913: Too many arguments (11/5)
> > > (too-many-arguments)
> > > tools/u_boot_pylib/gitutil.py:439:0: R0917: Too many positional
> > > arguments (11/5) (too-many-positional-arguments)
> > > tools/u_boot_pylib/gitutil.py:439:0: R0914: Too many local variables
> > > (16/15) (too-many-locals)
> > > tools/u_boot_pylib/gitutil.py:694:4: W0603: Using the global statement
> > > (global-statement)
> > >
> > > ------------------------------------------------------------------
> > > Your code has been rated at 9.73/10 (previous run: 8.41/10, +1.32)
> >
> > And it's not clear where these warnings come from or why they matter or
> > how to check things further.
> 
> Quite a few of them have been there for a while, but most of them are
> coming from newer versions of pylint, where people come up with new
> warnings. The f-string thing is the main one, since that feature was
> not available when the code was written, but now you get a warning if
> you don't use it, because Python.

OK. But pylint_err isn't complaining.

> > > The patch is here:
> > >
> > > https://patchwork.ozlabs.org/project/uboot/patch/20250209212714.2317040-2-sjg@chromium.org/
> > >
> > > It looks like it will apply for you, if first you accept my PR.
> >
> > It does, but I'm still not thrilled with the commit message.
> 
> I could do a new one with the info above if you like. We are going to
> have this all over the place...

Are we going to have to? If it's not an error I don't know how much
effort *needs* to be put in to it.

So yes, please provide a much clearer commit message for this patch.
Nothing is complaining until you ask a tool to give a complaint report,
so it's not nearly as obviously critical as "CI fails now" or "Everyone
gets spammed by python when using buildman".

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20250315/b2d66a7b/attachment-0001.sig>