Re: [Tarantool-patches] [PATCH luajit 15/15] test: run flake8 static analysis via CMake

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Igor!
Thanks for the fixes!
LGTM. Also, see my comments below.

On Tue, Aug 08, 2023 at 07:29:47PM +0000, Igor Munkin wrote:
> 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
> 
> ================================================================================
Awesome! Thanks for the fixes!
> 
> > > +  # 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?

Personally, I see no reason to postpone it and wait some for some kind
of 'refactoring' patch. However, if you believe it'll make the history
more consistent, then feel free to ignore.
> 
> > > +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