Tarantool development patches archive
 help / color / mirror / Atom feed
From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Maxim Kokryashkin <m.kokryashkin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit 15/15] test: run flake8 static analysis via CMake
Date: Tue, 8 Aug 2023 19:29:47 +0000	[thread overview]
Message-ID: <ZNKXqyzRbPEuC3QG@tarantool.org> (raw)
In-Reply-To: <r54ea56wyusxmetfhmj6jiyacxpdbauprkvisheu5risswka3p@ogzkeqo2qtpi>

Max,

Thanks for your review! See my answers inline.

On 04.08.23, Maxim Kokryashkin wrote:
> Hi, Igor!
> Thanks for the patch!
> 
> Aside from comments left by Sergey, please consider a few of mine.
> 
> On Thu, Aug 03, 2023 at 07:30:40AM +0000, Igor Munkin wrote:
> > This patch introduces a separate target to run flake8 against all Python
> > scripts within LuaJIT repository (i.e. debugger extensions). There are
> > some tweaks in .flake8rc regarding our style: one can find more info in
> > the config file.
> > 
> > The new target is a dependency for the new <LuaJIT-lint> target, that
> > joins both luacheck and flake8 linter runs. CI job with linters is
> > adjusted respectively.
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> >  .flake8rc                  | 12 ++++++++++++
> >  .github/workflows/lint.yml |  4 ++--
> >  test/CMakeLists.txt        | 28 ++++++++++++++++++++++++++++
> >  3 files changed, 42 insertions(+), 2 deletions(-)
> >  create mode 100644 .flake8rc
> > 
> > diff --git a/.flake8rc b/.flake8rc
> > new file mode 100644
> > index 00000000..b6f7ad48
> > --- /dev/null
> > +++ b/.flake8rc
> > @@ -0,0 +1,12 @@
> > +[flake8]
> > +extend-ignore =
> > +  # TODO: I have no idea, how to fix this. flake8 suggests nothing
> > +  # (like clang-format or checkpatch.pl do). Help needed.
> > +  E131,
> > +  # TODO: I have no idea, how to fix this. flake8 suggests nothing
> > +  # (like clang-format or checkpatch.pl do). Help needed.
> > +  E501,
> I dont't think we should disable the continuation line alignment rule (E131),
> which has explanation in the docs[1] and the line length limit[2] (E501), which is
> easily fixed.

*"I have no idea" why I decided to write that E131 is hard to fix...*
Maybe it's a brain damage as a result of solar flare, dunno.

All in all, I've marked all spots reported by flake8 with # noqa label
and here is why: All of the "misaligned" places are located deeply
inside a cascade of the calls and list comprehensions, so indenting the
tail of the comprehension with the same indent level as its beginning
makes the code barely readable at least for me. Yes, I have read enough
Python code and recently faced some sources with the "right"
indentation: it looks unreadable as fuck, since the only difference
between prefix and postfix <for> "statement" is the freaking colon. IMHO
the current indentation emphases the fact that this is a oneline
statement split into two lines due to the width limits. I know that
syntax error will occur if prefix variant is used instead of postfix and
vice versa, but we're talking about code formatting and readability. I
removed the global suppression and added inline suppressions in scope of
the separate commit (will send in reply to this message).

As for E501, I also rewrote some parts and finally fixed all occurrences
(will also send the patch in reply to this message).

New CI results here: https://github.com/tarantool/luajit/commit/f9c3849.
The diff for .flake8rc as a result of the aforementioned changes:

================================================================================

diff --git a/.flake8rc b/.flake8rc
index b6f7ad48..13e6178f 100644
--- a/.flake8rc
+++ b/.flake8rc
@@ -1,12 +1,5 @@
 [flake8]
 extend-ignore =
-  # TODO: I have no idea, how to fix this. flake8 suggests nothing
-  # (like clang-format or checkpatch.pl do). Help needed.
-  E131,
-  # TODO: I have no idea, how to fix this. flake8 suggests nothing
-  # (like clang-format or checkpatch.pl do). Help needed.
-  E501,
   # XXX: Suppress F821, since we have autogenerated names for
   # 'ptr' type complements in luajit_lldb.py.
   F821
-max-line-length = 80

================================================================================

> > +  # XXX: Suppress F821, since we have autogenerated names for
> > +  # 'ptr' type complements in luajit_lldb.py.
> > +  F821
> > +max-line-length = 80

<snipped>

> > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> > index 47296a22..17ac5cac 100644
> > --- a/test/CMakeLists.txt
> > +++ b/test/CMakeLists.txt
> > @@ -42,6 +42,34 @@ else()
> >    )
> >  endif()
> >  
> > +find_program(FLAKE8 flake8)
> > +if(FLAKE8)
> > +  get_filename_component(FLAKE8_SOURCE_DIR "${PROJECT_SOURCE_DIR}" REALPATH)
> > +  set(FLAKE8_RC ${FLAKE8_SOURCE_DIR}/.flake8rc)
> > +  file(GLOB_RECURSE FLAKE8_DEPS ${FLAKE8_SOURCE_DIR}/*.py)
> > +  add_custom_target(${PROJECT_NAME}-flake8
> > +    DEPENDS ${FLAKE8_DEPS}
> > +  )
> > +  add_custom_command(TARGET ${PROJECT_NAME}-flake8
> > +    COMMENT "Running flake8 static analysis"
> > +    COMMAND
> > +      ${FLAKE8} ${FLAKE8_DEPS}
> > +        --config ${FLAKE8_RC}
> > +        --jobs ${CMAKE_BUILD_PARALLEL_LEVEL}
> > +    WORKING_DIRECTORY ${FLAKE8_SOURCE_DIR}
> > +  )
> I suggest moving this logic to a separate cmake module. It will
> make the test/CMakeLists.txt cleaner and more readable.

Nice idea, thanks! However, I suggest to move it in scope of the series
integrating CTest (along with luacheck part). Does this work for you?

> > +else()
> > +  add_custom_target(${PROJECT_NAME}-flake8)
> > +  add_custom_command(TARGET ${PROJECT_NAME}-flake8
> > +    COMMENT "`flake8' is not found, so ${PROJECT_NAME}-flake8 target is dummy"
> > +  )
> > +endif()
> > +
> > +add_custom_target(${PROJECT_NAME}-lint DEPENDS
> > +  ${PROJECT_NAME}-luacheck
> > +  ${PROJECT_NAME}-flake8
> > +)
> > +
> >  set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
> >  separate_arguments(LUAJIT_TEST_COMMAND)
> >  
> > -- 
> > 2.30.2
> > 
> [1]: https://www.flake8rules.com/rules/E131.html
> [2]: https://www.flake8rules.com/rules/E501.html
> 
> Best regards,
> Maxim Kokryashkin

-- 
Best regards,
IM

  reply	other threads:[~2023-08-08 19:43 UTC|newest]

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-03  7:30 [Tarantool-patches] [PATCH luajit 00/15] Add flake8 linter Igor Munkin via Tarantool-patches
2023-08-03  7:30 ` [Tarantool-patches] [PATCH luajit 01/15] test: fix E122 errors by pycodestyle Igor Munkin via Tarantool-patches
2023-08-03 14:25   ` Sergey Bronnikov via Tarantool-patches
2023-08-03 15:49   ` Maxim Kokryashkin via Tarantool-patches
2023-08-03  7:30 ` [Tarantool-patches] [PATCH luajit 02/15] test: fix E128 " Igor Munkin via Tarantool-patches
2023-08-03 14:26   ` Sergey Bronnikov via Tarantool-patches
2023-08-03 15:52   ` Maxim Kokryashkin via Tarantool-patches
2023-08-03  7:30 ` [Tarantool-patches] [PATCH luajit 03/15] test: fix E201 and E202 " Igor Munkin via Tarantool-patches
2023-08-03 14:26   ` Sergey Bronnikov via Tarantool-patches
2023-08-03 15:53   ` Maxim Kokryashkin via Tarantool-patches
2023-08-03  7:30 ` [Tarantool-patches] [PATCH luajit 04/15] test: fix E203 " Igor Munkin via Tarantool-patches
2023-08-03 14:26   ` Sergey Bronnikov via Tarantool-patches
2023-08-03 15:55   ` Maxim Kokryashkin via Tarantool-patches
2023-08-03  7:30 ` [Tarantool-patches] [PATCH luajit 05/15] test: fix E231 " Igor Munkin via Tarantool-patches
2023-08-03 14:26   ` Sergey Bronnikov via Tarantool-patches
2023-08-03 15:55   ` Maxim Kokryashkin via Tarantool-patches
2023-08-03  7:30 ` [Tarantool-patches] [PATCH luajit 06/15] test: fix E251 " Igor Munkin via Tarantool-patches
2023-08-03 14:27   ` Sergey Bronnikov via Tarantool-patches
2023-08-03 15:58   ` Maxim Kokryashkin via Tarantool-patches
2023-08-03  7:30 ` [Tarantool-patches] [PATCH luajit 07/15] test: fix E301 " Igor Munkin via Tarantool-patches
2023-08-03 14:28   ` Sergey Bronnikov via Tarantool-patches
2023-08-03 16:01   ` Maxim Kokryashkin via Tarantool-patches
2023-08-03  7:30 ` [Tarantool-patches] [PATCH luajit 08/15] test: fix E302 " Igor Munkin via Tarantool-patches
2023-08-03 14:28   ` Sergey Bronnikov via Tarantool-patches
2023-08-03 16:02   ` Maxim Kokryashkin via Tarantool-patches
2023-08-03  7:30 ` [Tarantool-patches] [PATCH luajit 09/15] test: fix E303 " Igor Munkin via Tarantool-patches
2023-08-03 14:28   ` Sergey Bronnikov via Tarantool-patches
2023-08-03 16:03   ` Maxim Kokryashkin via Tarantool-patches
2023-08-03  7:30 ` [Tarantool-patches] [PATCH luajit 10/15] test: fix E305 " Igor Munkin via Tarantool-patches
2023-08-03 14:28   ` Sergey Bronnikov via Tarantool-patches
2023-08-03 16:05   ` Maxim Kokryashkin via Tarantool-patches
2023-08-03  7:30 ` [Tarantool-patches] [PATCH luajit 11/15] test: fix E502 " Igor Munkin via Tarantool-patches
2023-08-03 14:29   ` Sergey Bronnikov via Tarantool-patches
2023-08-03 16:06   ` Maxim Kokryashkin via Tarantool-patches
2023-08-03  7:30 ` [Tarantool-patches] [PATCH luajit 12/15] test: fix E711 " Igor Munkin via Tarantool-patches
2023-08-03 14:29   ` Sergey Bronnikov via Tarantool-patches
2023-08-03 16:06   ` Maxim Kokryashkin via Tarantool-patches
2023-08-03  7:30 ` [Tarantool-patches] [PATCH luajit 13/15] test: fix E722 " Igor Munkin via Tarantool-patches
2023-08-03 14:29   ` Sergey Bronnikov via Tarantool-patches
2023-08-03 16:10   ` Maxim Kokryashkin via Tarantool-patches
2023-08-03  7:30 ` [Tarantool-patches] [PATCH luajit 14/15] test: fix E741 " Igor Munkin via Tarantool-patches
2023-08-03 14:34   ` Sergey Bronnikov via Tarantool-patches
2023-08-07 11:00     ` Igor Munkin via Tarantool-patches
2023-08-07 13:45       ` Sergey Bronnikov via Tarantool-patches
2023-08-03 16:15   ` Maxim Kokryashkin via Tarantool-patches
2023-08-07 10:57     ` Igor Munkin via Tarantool-patches
2023-08-13 20:25       ` Maxim Kokryashkin via Tarantool-patches
2023-08-03  7:30 ` [Tarantool-patches] [PATCH luajit 15/15] test: run flake8 static analysis via CMake Igor Munkin via Tarantool-patches
2023-08-03 14:23   ` Sergey Bronnikov via Tarantool-patches
2023-08-03 14:25     ` Sergey Bronnikov via Tarantool-patches
2023-08-07 13:35       ` Igor Munkin via Tarantool-patches
2023-08-07 13:41         ` [Tarantool-patches] [PATCH luajit 16/15] gdb: fix Python <assert> statement usage Igor Munkin via Tarantool-patches
2023-08-08  8:26           ` Sergey Bronnikov via Tarantool-patches
2023-08-13 20:24           ` Maxim Kokryashkin via Tarantool-patches
2023-08-07 13:41         ` [Tarantool-patches] [PATCH luajit 17/15] test: fix E275 errors by pycodestyle Igor Munkin via Tarantool-patches
2023-08-08  8:26           ` Sergey Bronnikov via Tarantool-patches
2023-08-13 19:25           ` Maxim Kokryashkin via Tarantool-patches
2023-08-08  8:18         ` [Tarantool-patches] [PATCH luajit 15/15] test: run flake8 static analysis via CMake Sergey Bronnikov via Tarantool-patches
2023-08-07 12:17     ` Igor Munkin via Tarantool-patches
2023-08-07 13:48       ` Sergey Bronnikov via Tarantool-patches
2023-08-03 21:02   ` Maxim Kokryashkin via Tarantool-patches
2023-08-08 19:29     ` Igor Munkin via Tarantool-patches [this message]
2023-08-08 19:42       ` [Tarantool-patches] [PATCH luajit 18/15] test: suppress E131 errors by pycodestyle Igor Munkin via Tarantool-patches
2023-08-13 13:52         ` Maxim Kokryashkin via Tarantool-patches
2023-08-08 19:42       ` [Tarantool-patches] [PATCH luajit 19/15] test: fix E501 " Igor Munkin via Tarantool-patches
2023-08-13 13:55         ` Maxim Kokryashkin via Tarantool-patches
2023-08-14  7:28       ` [Tarantool-patches] [PATCH luajit 15/15] test: run flake8 static analysis via CMake Maxim Kokryashkin via Tarantool-patches
2023-08-21 11:05 ` [Tarantool-patches] [PATCH luajit 00/15] Add flake8 linter Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZNKXqyzRbPEuC3QG@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=m.kokryashkin@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit 15/15] test: run flake8 static analysis via CMake' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox