From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id C0E65599C4A; Mon, 14 Aug 2023 10:28:24 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C0E65599C4A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1691998104; bh=0IZTsyx+kYclFejTowR0t0yYYLuBwzUdCvtUvdyOZq4=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=Oq8KGm2uwInelS2XaXjowzu+aScYi+W/gaxDLGv7Miaq235fHpXBDRz1YKbCTYWTC l7QbwvCwcIYWa1n5xvTrpFJqYFIC6WmSt8oh1043kblF2UJKGL3q8pAhu80Vh/WhYD a7w8W09uyZnsj9AlWsIuqZxaelqhG5UtKwrO/FR4= Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [95.163.41.84]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id BF44F45D303 for ; Mon, 14 Aug 2023 10:28:23 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BF44F45D303 Received: by smtp46.i.mail.ru with esmtpa (envelope-from ) id 1qVRzy-0006Pn-0O; Mon, 14 Aug 2023 10:28:23 +0300 Date: Mon, 14 Aug 2023 10:28:20 +0300 To: Igor Munkin Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD969E04B5EED670DC83148C852D424A0A0119F8DAA2A6AAA9D182A05F538085040CD0ECB6A0F6A87A43856ACFDB126A471F4DBC21C047D28A1DC23308702CB2CB5 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE76AB1B6FB25ACEDC9EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637B84F9009663064BD8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8721840FD5C5358311925E3B3E9F47311117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC0F49EF363AAD6E82A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD1828451B159A507268D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EEB28585415E75ADA9CEDA8D6C8C3B0531D8FC6C240DEA76429C9F4D5AE37F343AA9539A8B242431040A6AB1C7CE11FEE367F1C1C3ABB44F3A6E0066C2D8992A16C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407978DA827A17800CE7649B83402744A6742DBA43225CD8A89F83C798A30B85E16B156CCFE7AF13BCA4B5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5BCAE27CA0C8D6221F1718B78EA1386E3A88BB39C071A5889F87CCE6106E1FC07E67D4AC08A07B9B06A1CB4668A9CA5FACB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D3455AC8BF8E3153BA072ECDF123192943723A6C3467A419B8130D1690D666093484B4CBCBB518978EF1D7E09C32AA3244C953DBD9AFC33E78EA8ACE8B5834C15C464EE5813BBCA3A9D85A42E4C463514DC5DA084F8E80FEBD396F07DFE06A4A8314E894E437E78228B66933FA05BD8EF0CAD958392AE682691 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojngxRbPBK1nEjjRYMkC+rNA== X-Mailru-Sender: 0E9E14D9EC491FBA30DF66BD174AD530F9E2244D9D5CFEA43856ACFDB126A4717958A49548C749F604C9FB44FCBCE9EE92D99EB8CC7091A7ECEABDC5717908DEF544888E8238EB4872D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 15/15] test: run flake8 static analysis via CMake X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Maxim Kokryashkin via Tarantool-patches Reply-To: Maxim Kokryashkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 target, that > > > joins both luacheck and flake8 linter runs. CI job with linters is > > > adjusted respectively. > > > > > > Signed-off-by: Igor Munkin > > > --- > > > .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 "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 > > > > > > 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