From 9918cb3335616e55f0b774265a53ba75f3beee30 Mon Sep 17 00:00:00 2001 From: Andrey Aksenov <38073144+andreyaksenov@users.noreply.github.com> Date: Fri, 11 Aug 2023 15:03:15 +0300 Subject: [PATCH] Fix feedback issues (#3614) --- doc/concepts/coop_multitasking.rst | 2 +- doc/dev_guide/lua_style_guide.rst | 150 +++++++++++--------- doc/reference/tooling/luajit_getmetrics.rst | 4 +- doc/reference/tooling/luajit_memprof.rst | 8 +- styles/Tarantool/ComplexWords.yml | 10 -- styles/Tarantool/WordList.yml | 1 - styles/Vocab/Tarantool/accept.txt | 17 ++- 7 files changed, 106 insertions(+), 86 deletions(-) diff --git a/doc/concepts/coop_multitasking.rst b/doc/concepts/coop_multitasking.rst index dc722c6b65..c5ae8bbcc9 100644 --- a/doc/concepts/coop_multitasking.rst +++ b/doc/concepts/coop_multitasking.rst @@ -54,7 +54,7 @@ transfers control of the thread from the current fiber to another fiber that is Any live fiber can be in one of three states: ``running``, ``suspended``, and ``ready``. After a fiber dies, the ``dead`` status is returned. By observing fibers from the outside, you can only see ``running`` (for the current fiber) -and ``suspended`` for any other fiber waiting for an event from eventloop (``ev``) +and ``suspended`` for any other fiber waiting for an event from the event loop (``ev``) for execution. .. image:: yields.svg diff --git a/doc/dev_guide/lua_style_guide.rst b/doc/dev_guide/lua_style_guide.rst index 6a6bad3a9c..d70af67684 100644 --- a/doc/dev_guide/lua_style_guide.rst +++ b/doc/dev_guide/lua_style_guide.rst @@ -65,16 +65,19 @@ Indentation and formatting .. code-block:: lua + -- Good if (a == true and b == false) or (a == false and b == true) then <...> - end -- good + end + -- Bad if a == true and b == false or a == false and b == true then <...> - end -- bad + end + -- Good but not explicit if a ^ b == true then - end -- good, but not explicit + end * Type conversion @@ -83,31 +86,32 @@ Indentation and formatting .. code-block:: lua + -- Bad local a = 123 a = a .. '' - -- bad + -- Good local a = 123 a = tostring(a) - -- good + -- Bad local a = '123' a = a + 5 -- 128 - -- bad + -- Good local a = '123' a = tonumber(a) + 5 -- 128 - -- good * Try to avoid multiple nested ``if``'s with common body: .. code-block:: lua + -- Good if (a == true and b == false) or (a == false and b == true) then do_something() end - -- good + -- Bad if a == true then if b == false then do_something() @@ -117,56 +121,59 @@ Indentation and formatting do_something() end end - -- bad * Avoid multiple concatenations in one statement, use ``string.format`` instead: .. code-block:: lua + -- Bad function say_greeting(period, name) local a = "good " .. period .. ", " .. name end - -- bad + -- Good function say_greeting(period, name) local a = string.format("good %s, %s", period, name) end - -- good + -- Best local say_greeting_fmt = "good %s, %s" function say_greeting(period, name) local a = say_greeting_fmt:format(period, name) end - -- best * Use ``and``/``or`` for default variable values .. code-block:: lua + -- Good function(input) input = input or 'default_value' - end -- good + end + -- Ok but excessive function(input) if input == nil then input = 'default_value' end - end -- ok, but excessive + end * ``if``'s and return statements: .. code-block:: lua + -- Good if a == true then return do_something() end - do_other_thing() -- good + do_other_thing() + -- Bad if a == true then return do_something() else do_other_thing() - end -- bad + end * Using spaces: @@ -175,22 +182,24 @@ Indentation and formatting .. code-block:: lua + -- Bad function name (arg1,arg2,...) - end -- bad + end + -- Good function name(arg1, arg2, ...) - end -- good + end - Add a space after comment markers: .. code-block:: lua - while true do -- inline comment - -- comment - do_something() + while true do -- Inline comment + -- Comment + do_something() end --[[ - multiline + Multiline comment ]]-- @@ -198,46 +207,46 @@ Indentation and formatting .. code-block:: lua + -- Bad local thing=1 thing = thing-1 thing = thing*1 thing = 'string'..'s' - -- bad + -- Good local thing = 1 thing = thing - 1 thing = thing * 1 thing = 'string' .. 's' - -- good - Add a space after commas in tables: .. code-block:: lua + -- Bad local thing = {1,2,3} thing = {1 , 2 , 3} thing = {1 ,2 ,3} - -- bad + -- Good local thing = {1, 2, 3} - -- good - Add a space in map definitions after equals signs and commas: .. code-block:: lua + -- Bad return {1,2,3,4} return { key1 = val1,key2=val2 } - -- bad + -- Good return {1, 2, 3, 4} return { key1 = val1, key2 = val2, - key3 = vallll + key3 = val3 } - -- good You can also use alignment: @@ -258,25 +267,25 @@ Indentation and formatting .. code-block:: lua + -- Bad if thing ~= nil then - -- ...stuff... + -- ... stuff ... end function derp() - -- ...stuff... + -- ... stuff ... end local wat = 7 - -- bad + -- Good if thing ~= nil then - -- ...stuff... + -- ... stuff ... end function derp() - -- ...stuff... + -- ... stuff ... end local wat = 7 - -- good - Delete whitespace at EOL (strongly forbidden. Use ``:s/\s\+$//gc`` in vim to delete them). @@ -290,14 +299,18 @@ add a prefix, or add a table instead of a prefix: .. code-block:: lua + -- Very bad function bad_global_example() - end -- very, very bad + end function good_local_example() end - _G.modulename_good_local_example = good_local_example -- good + -- Good + _G.modulename_good_local_example = good_local_example + + -- Better _G.modulename = {} - _G.modulename.good_local_example = good_local_example -- better + _G.modulename.good_local_example = good_local_example Always use a prefix to avoid name conflicts. @@ -369,11 +382,11 @@ To write modules, use one of the two patterns (don't use ``modules()``): local M = {} function M.foo() - ... + ... end function M.bar() - ... + ... end return M @@ -383,16 +396,16 @@ or .. code-block:: lua local function foo() - ... + ... end local function bar() - ... + ... end return { - foo = foo, - bar = bar, + foo = foo, + bar = bar, } Commenting @@ -401,6 +414,8 @@ Commenting Don't forget to comment your Lua code. You shouldn't comment Lua syntax (assume that the reader already knows the Lua language). Instead, tell about functions/variable names/etc. +Start a sentence with a capital letter and end with a period. + Multiline comments: use matching (``--[[ ]]--``) instead of simple (``--[[ ]]``). @@ -408,10 +423,10 @@ Public function comments: .. code-block:: lua - --- Copy any table (shallow and deep version) + --- Copy any table (shallow and deep version). -- * deepcopy: copies all levels -- * shallowcopy: copies only first level - -- Supports __copy metamethod for copying custom tables with metatables + -- Supports __copy metamethod for copying custom tables with metatables. -- @function gsplit -- @table inp original table -- @shallow[opt] sep flag for shallow copy @@ -429,43 +444,44 @@ Use the ``tap`` module for writing efficient tests. Example of a test file: local test = require('tap').test('table') test:plan(31) - do -- check basic table.copy (deepcopy) + do + -- Check basic table.copy (deepcopy). local example_table = { - {1, 2, 3}, - {"help, I'm very nested", {{{ }}} } + { 1, 2, 3 }, + { "help, I'm very nested", { { { } } } } } local copy_table = table.copy(example_table) test:is_deeply( - example_table, - copy_table, - "checking, that deepcopy behaves ok" + example_table, + copy_table, + "checking, that deepcopy behaves ok" ) test:isnt( - example_table, - copy_table, - "checking, that tables are different" + example_table, + copy_table, + "checking, that tables are different" ) test:isnt( - example_table[1], - copy_table[1], - "checking, that tables are different" + example_table[1], + copy_table[1], + "checking, that tables are different" ) test:isnt( - example_table[2], - copy_table[2], - "checking, that tables are different" + example_table[2], + copy_table[2], + "checking, that tables are different" ) test:isnt( - example_table[2][2], - copy_table[2][2], - "checking, that tables are different" + example_table[2][2], + copy_table[2][2], + "checking, that tables are different" ) test:isnt( - example_table[2][2][1], - copy_table[2][2][1], - "checking, that tables are different" + example_table[2][2][1], + copy_table[2][2][1], + "checking, that tables are different" ) end diff --git a/doc/reference/tooling/luajit_getmetrics.rst b/doc/reference/tooling/luajit_getmetrics.rst index 8fb026c181..c1ab6310ef 100644 --- a/doc/reference/tooling/luajit_getmetrics.rst +++ b/doc/reference/tooling/luajit_getmetrics.rst @@ -84,7 +84,7 @@ Some of the table members shown here are used in the examples that come later in +----------------------+--------------------------------------------------+------------+ | gc_steps_sweep | number of steps of garbage collector, | yes | | | sweep phases | | - | | See external `Sweep Phase Description`_ | | + | | (see the `Sweep phase description`_) | | +----------------------+--------------------------------------------------+------------+ | gc_steps_sweepstring | number of steps of garbage collector, | yes | | | sweep phases for strings | | @@ -118,7 +118,7 @@ Some of the table members shown here are used in the examples that come later in .. comment: Links are not inline because they would make the table cells wider. -.. _Sweep phase description: http://wiki.luajit.org/New-Garbage-Collector#sweep-phase +.. _Sweep phase description: https://ujit.readthedocs.io/en/latest/public/tut-new-gc.html#sweep-phase .. _Snap tutorial: https://ujit.readthedocs.io/en/latest/public/tut-snap.html Note: Although value names are similar to value names in diff --git a/doc/reference/tooling/luajit_memprof.rst b/doc/reference/tooling/luajit_memprof.rst index 2c2445ff7a..87a793e9f5 100644 --- a/doc/reference/tooling/luajit_memprof.rst +++ b/doc/reference/tooling/luajit_memprof.rst @@ -36,7 +36,7 @@ you need to place this part between two ``misc.memprof`` functions, namely, ``misc.memprof.start()`` and ``misc.memprof.stop()``, and then execute the code under Tarantool. -Below is a chunk of simple Lua code named ``test.lua`` to illustrate this. +Below is a chunk of Lua code named ``test.lua`` to illustrate this. .. _profiler_usage_example01: @@ -135,7 +135,7 @@ Parsing binary profile and generating profiling report After getting the memory profile in binary format, the next step is to parse it to get a human-readable profiling report. You can do this via Tarantool by using the following command -(mind the hyphen ``-`` prior to the file name): +(mind the hyphen ``-`` before the filename): .. code-block:: console @@ -425,7 +425,7 @@ Reasonable questions regarding the report can be: * Why are there no allocations related to the ``concat()`` function? * Why is the number of allocations not a round number? -* Why are there approximately 20K allocations instead of 10K? +* Why are there about 20K allocations instead of 10K? First of all, LuaJIT doesn't create a new string if the string with the same payload exists (see details on `lua-users.org/wiki `_). @@ -447,7 +447,7 @@ You can see the difference in behavior by adding the line ``local _ = tostring(i)`` between lines 22 and 23. To profile only the ``concat()`` function, comment out line 23 (which is -``local f = format(i)``) and run the profiler. Now the output will look like this: +``local f = format(i)``) and run the profiler. Now the output looks like this: .. code-block:: console diff --git a/styles/Tarantool/ComplexWords.yml b/styles/Tarantool/ComplexWords.yml index 6b586b2092..ac2db22bbe 100644 --- a/styles/Tarantool/ComplexWords.yml +++ b/styles/Tarantool/ComplexWords.yml @@ -17,8 +17,6 @@ swap: accurate: right|exact acquiesce: agree acquire: get|buy - additional: more|extra - address: discuss addressees: you adjacent to: next to adjustment: change @@ -46,7 +44,6 @@ swap: collaborate: work together commence: begin compensate: pay - component: part comprise: form|include concept: idea concerning: about @@ -68,7 +65,6 @@ swap: elucidate: explain employ: use enclosed: inside|included - encounter: meet endeavor: try enumerate: count equitable: fair @@ -77,10 +73,8 @@ swap: expedite: hurry facilitate: ease females: women - finalize: complete|finish frequently: often identical: same - incorrect: wrong indication: sign initiate: start|begin itemized: listed @@ -91,7 +85,6 @@ swap: modify: change monitor: check|watch necessitate: cause - notify: tell numerous: many objective: aim|goal obligate: bind|compel @@ -99,11 +92,8 @@ swap: permit: let portion: part possess: own - previous: earlier previously: before - prioritize: rank procure: buy - provide: give|offer purchase: buy relocate: move solicit: request diff --git a/styles/Tarantool/WordList.yml b/styles/Tarantool/WordList.yml index 47344dcd45..3762372475 100644 --- a/styles/Tarantool/WordList.yml +++ b/styles/Tarantool/WordList.yml @@ -24,7 +24,6 @@ swap: 'Google (?:I\-O|IO)': Google I/O "tap (?:&|and) hold": touch & hold "un(?:check|select)": clear - above: preceding account name: username action bar: app bar admin: administrator diff --git a/styles/Vocab/Tarantool/accept.txt b/styles/Vocab/Tarantool/accept.txt index 68a95dfb66..87defe87e3 100644 --- a/styles/Vocab/Tarantool/accept.txt +++ b/styles/Vocab/Tarantool/accept.txt @@ -17,4 +17,19 @@ nullable coroutines multikey datetime -varbinary \ No newline at end of file +varbinary +stdin +stdout +stderr +metatable +multiline +profiler +vararg +finalizer +cdata +userdata +coroutine +storages +dereference +upsert +tt \ No newline at end of file