Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust the existing Lua style guide #1004

Open
igormunkin opened this issue Nov 27, 2019 · 11 comments
Open

Adjust the existing Lua style guide #1004

igormunkin opened this issue Nov 27, 2019 · 11 comments
Labels
dev To be updated by the developers. Includes developer guidelines and many API elements. guidelines

Comments

@igormunkin
Copy link
Collaborator

Considering the existing Lua style guide there is no mention for our test naming policy. This led to a little arguing in our patch list.

@Gerold103 claimed that we agreed to the following some time ago:

gh-####-description.test.lua

However, since we backport bugs related to the issues from other queues, the gh-#### prefix is ambigious. Thereby I guess the following pattern is the most convenient one for LuaJIT related tests:

<origin-issue-repo>-####-description.test.lua

Here are some examples:

  • LuaJIT-505-fold-bug-in-string-find.test.lua
  • tarantool-3196-bug-with-zero-hash-strings.test.lua

@kyukhin, @Totktonada, @Gerold103, @kostja, please dump your thoughts related to the naming policy here for the further Lua style guide amending.


Furthermore, all existing tests in LuaJIT suite seem to be written considering the current guideline and violate several practices mentioned by @Totktonada in the review:

  • Use local for variables (tap, test).
  • Use os.exit(test:check() and 0 or 1) at the end.

Please consider applying these comments to the example in out style guide.

@lenkis
Copy link
Contributor

lenkis commented Dec 9, 2019

Waiting for input from @kyukhin, @Totktonada, @Gerold103, @kostja

@lenkis lenkis added 1.10 need feedback [special status] On hold, awaiting feedback labels Dec 9, 2019
@lenkis lenkis self-assigned this Dec 9, 2019
@Totktonada
Copy link
Member

We discussed it voicely with @igormunkin already, I'm okay.

How many persons are necessary to form a quorum here?

@Gerold103
Copy link
Contributor

I am against so long names. Test-run barely can print gh-####-descr.test.lua without screwing the output.
Also I am against <origin-issue-repo>. We use 'gh' not because of repo. 'gh' here means 'GitHub'. Repository is fixed. It is not an option to choose. All tests in tarantool/tarantool belong to tarantool repo. All tests in tarantool/small belong to small repo, and so one.

@Totktonada
Copy link
Member

@Gerold103 Are you propose to file an issue to tarantool/tarantool (or tarantool/luajit?) for each problem that is discussed within an LuaJIT / OpenResty / luavela / moonjit issue? Is it worth even if we'll just cherry-pick an existing commit from some of those repositories? Are there any other gains we'll get with this process other then good test-run output?

Are there any other problems with Igor's proposal if we'll adjust test-run output?

@Gerold103
Copy link
Contributor

I propose you to think more on the new naming schema, and fix the test-run output. At least you could come up with shorter names such as tnt instead of tarantool, lj instead of luajit etc.

@Totktonada
Copy link
Member

So hot discussion :)

I don't mind against short names, however personally I prefer to don't abbreviate things if possible, except maybe most common cases. Anyway, this is not a problem: test-run can be adjusted easily. Your opinion looks as a personal taste, just like my taste to avoid abbreviations.

I propose you to think more <...>

It was unclear what do you propose: if you have objections, then you should describe your variant, right?

@Gerold103
Copy link
Contributor

Well, I was asked to 'give input', and I gave it - I didn't like the proposed solution. After you asked what do I propose - I started thinking about it and gave a few ideas such as use shorter repo names.

@avtikhon
Copy link

avtikhon commented Dec 12, 2019

As about me, I'd prefer the following format:
p.s. tarantool repo by default, so let's use for it gh-<issue number>-...

gh-[repo: lj|or]-<issue number>-<short description>...

@lenkis lenkis assigned veod32 and unassigned lenkis Dec 25, 2019
@Totktonada
Copy link
Member

Observed during another review:

As far as I see, our tests tend to use gh-dddd-short-description.test.lua name pattern for issue-related cases and name_with_several_words.test.lua for subsystem-related cases.

https://lists.tarantool.org/pipermail/tarantool-patches/2020-January/013595.html

@kyukhin
Copy link
Contributor

kyukhin commented May 29, 2020

As about me, I'd prefer the following format:
p.s. tarantool repo by default, so let's use for it gh--...

gh-[repo: lj|or]-<issue number>-<short description>...

I like this idea. Let's make 2-letters optional non-tarantool repo reference.

Examples:

  1. none for Tarantool repo
  2. lj for vanilla Luajit repo
  3. uj for ujit repo
  4. mp for msgpuck repo

@Totktonada
Copy link
Member

Now we use the following convention within tarantool/luajit repository:

<repo>-<number>-<comment>.test.lua
where "<repo>" is github repository:
  gh - tarantool/tarantool
  lj - LuaJIT/LuaJIT
  or - openresty/luajit2

I guess the question is resolved: there is nothing more to discuss and we can finally update the documentation.

@lenkis Are there any questions here?

@kyukhin kyukhin removed the need feedback [special status] On hold, awaiting feedback label Sep 4, 2020
@patiencedaur patiencedaur removed the 1.10 label Apr 18, 2022
@patiencedaur patiencedaur added the dev To be updated by the developers. Includes developer guidelines and many API elements. label May 20, 2022
@veod32 veod32 removed their assignment Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev To be updated by the developers. Includes developer guidelines and many API elements. guidelines
Projects
None yet
Development

No branches or pull requests

8 participants