feat(descriptors, lua): add stylua support
#3985
Conversation
AlejandroSuero
requested review from
nvuillam,
echoix,
bdovaz and
Kurt-von-Laven
as code owners
|
It looks fine! It would've worked without the FROM too, but it's true that we didn't use that pattern yet here. But it's fine like this! |
|
Does stylua have sensible defaults without a template config file? |
|
@echoix I think it uses these default options without a config file. Running
And this in the current
|
|
I don't know what to conclude about the two screenshots, but I see that it is a linter that can work by itself with a config when not specified. |
|
@nvuillam can you confirm that the arguments to call are correct in the descriptor, especially when multiple config file names are possible and when no config is given? |
|
Ok, it seems that test files need to be adjusted (lua files). |
|
@echoix in what way do the tests need to be adjusted? Shouldn't |
|
@echoix I tried letting the command fail like: error: could not format file .automation/test/lua/lua_bad_1.lua: error parsing: error occurred while creating ast: unexpected token ``. (starting from line 10, character 1 and ending on line 10, character 1)
additional information: expected 'end'Instead of: Diff in .automation/test/lua/lua_bad_1.lua:
1 1 | package.loaded[...] = {}
2 2 |
3 3 | local function helper(...)
4 |- -- NYI
4 |+ -- NYI
5 5 | end
6 6 |
7 7 | function embrace(opt)
8 |- local opt = opt or "default"
9 |- return hepler(opt.."?")
8 |+ local opt = opt or "default"
9 |+ return hepler(opt .. "?")
10 10 | endTo see if that's the problem that is running into. |
|
If you want a faster feedback, you can create a PR from the same branch as this PR, but against your fork. It will run the builds of individual linters, which when at the lua stylua, will take somewhere between 30secs and 2 minutes to build and test with only that linter. |
|
About test failure: lua_bad_1.lua must be wrongly formatted, and lua formatter be called with something like --check |
|
For the stylua file not found at first, check that the folder copied to corresponds to a folder that is on path. That means to look in the full dockerfile in what folder binaries are copied to |
Is it because of this |
|
The last argument would be the destination, and would be a folder, not a file. You can specify multiple source files, but one destination. |
|
@echoix I figured, will do a PR on |
|
Is there multiple binaries to be copied to? Ah, well, it might also be working, as it doesn't end with a slash... let me reread. (Or if you have a locally built image, go log into it and explore the folder hierarchy to find out without guessing) |
|
An excerpt:
|
|
@echoix when trying to build it locally it errors with this output: But according to the DockerHub of stylua https://hub.docker.com/r/johnnymorganz/stylua/tags it exists there. |
|
I removed the version, letting only Dockerfile:71
--------------------
69 | FROM checkmarx/kics:${REPOSITORY_KICS_VERSION} AS kics
70 | FROM trufflesecurity/trufflehog:${REPOSITORY_TRUFFLEHOG_VERSION} AS trufflehog
71 | >>> FROM jdkato/vale:${SPELL_VALE_VERSION} AS vale
72 | FROM lycheeverse/lychee:latest-alpine AS lychee
73 | FROM ghcr.io/terraform-linters/tflint:v${TERRAFORM_TFLINT_VERSION} AS tflint
--------------------
ERROR: failed to solve: jdkato/vale:v3.7.0: no match for platform in manifest: not found |
|
I managed to get it to work using cargo install 04aaf36 and not using lua_stylua Job=================================== FAILURES ===================================
_______________________ lua_stylua_test.test_format_fix ________________________
[gw0] linux -- Python 3.12.5 /usr/local/bin/python
self = <lua_stylua_test.lua_stylua_test testMethod=test_format_fix>
def test_format_fix(self):
self.request_id = str(uuid.uuid1())
utilstest.linter_test_setup({"request_id": self.request_id})
linter = self.get_linter_instance(self.request_id)
linter.pre_test()
> utilstest.test_linter_format_fix(linter, self)
megalinter/tests/test_megalinter/LinterTestRoot.py:90:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
linter = <megalinter.Linter.Linter object at 0x7fd4d47024e0>
test_self = <lua_stylua_test.lua_stylua_test testMethod=test_format_fix>
def test_linter_format_fix(linter, test_self):
if (
linter.disabled is True
or "all" in getattr(linter, "descriptor_flavors_exclude", [])
or (linter.is_formatter is False and linter.cli_lint_fix_arg_name is None)
):
raise unittest.SkipTest("Linter doesn't format and can't apply fixes")
test_folder = linter.test_folder
workspace = (
config.get(linter.request_id, "DEFAULT_WORKSPACE") + os.path.sep + test_folder
)
# Special cases when files must be copied in a temp directory before being linted
if os.path.isdir(workspace + os.path.sep + "fix"):
workspace = workspace + os.path.sep + "fix"
tmp_report_folder = tempfile.gettempdir() + os.path.sep + str(uuid.uuid4())
assert os.path.isdir(workspace), f"Test folder {workspace} is not existing"
file_map = {}
search_glob_pattern = workspace.replace("\\", "/") + "/**/*"
files = glob.glob(search_glob_pattern, recursive=True)
filter_regex_exclude = []
if linter.test_format_fix_regex_exclude is not None:
filter_regex_exclude = [linter.test_format_fix_regex_exclude]
file_extensions = linter.file_extensions
if len(linter.test_format_fix_file_extensions) > 0:
file_extensions = linter.test_format_fix_file_extensions
filtered_files = utils.filter_files(
all_files=files,
filter_regex_include="_fix_",
filter_regex_exclude=filter_regex_exclude,
file_names_regex=[],
file_extensions=file_extensions,
ignored_files=[],
ignore_generated_files=False,
)
for file in filtered_files:
with open(file, "r", encoding="utf-8") as f_expected:
content_expected = f_expected.read()
file_map[file] = content_expected
if len(file_map) == 0:
> raise Exception(f"[test] No files found in: {workspace}")
E Exception: [test] No files found in: /tmp/lint/.automation/test/luaNote All of this was tested in AlejandroSuero#2 |
You must be working on a M1 macOS right? In that case trying locally might only bring more problems than it's worth |
|
@echoix yeah, I'm using a M1, but trying it on the branch, for some reason |
|
@AlejandroSuero it might be a bug on stylia side ^^ |
|
I looked at their docker image, they use distroless-cc as production image, meaning that it is a minimal glibc binary, not muslc. The problem might be there. But stylua is also packaged on apk in the community channel (we don't use it I think). |
|
@nvuillam I'll point it to them in an issue. On thing that failed in the last commit is that Build & Deploy - Dev output------------------------------ Captured log call -------------------------------
DEBUG root:Linter.py:477 [Activation] + LUA_STYLUA (LUA) was activated by default strategies
DEBUG root:Linter.py:1179 Linter help command: ['stylua', '-h']
DEBUG root:Linter.py:1194 Linter help result: 0 stylua 0.20.0
A utility to format Lua code
USAGE:
stylua [OPTIONS] [FILES]...
ARGS:
<FILES>... A list of files to format
OPTIONS:
-a, --allow-hidden
Whether to traverse hidden files/directories
-c, --check
Runs in 'check' mode
--color <COLOR>
Use colored output [default: Auto] [possible values: Always, Auto, Never]
-f, --config-path <CONFIG_PATH>
Specify path to stylua.toml configuration file
-g, --glob <GLOB>
Glob patterns to test against which files to check
-h, --help
Print help information
--no-editorconfig
Disables the EditorConfig feature
--num-threads <NUM_THREADS>
The number of threads to use to format files in parallel [default: 4]
--output-format <OUTPUT_FORMAT>
Configures the diff output when using 'check' mode [default: Standard] [possible values:
Standard, Unified, Json, Summary]
--range-end <RANGE_END>
An ending range to format files, given as a byte offset from the beginning of the file
--range-start <RANGE_START>
A starting range to format files, given as a byte offset from the beginning of the file
--respect-ignores
Respect .styluaignore and glob matching for file paths provided directly to the tool
-s, --search-parent-directories
Search parent directories for stylua.toml, if not found in current directory. Ignored if
config_path is provided
--stdin-filepath <STDIN_FILEPATH>
Specify the location of the file that is being passed into stdin. Ignored if not taking
in input from stdin
-v, --verbose
Enables verbose output
-V, --version
Print version information
--verify
Verifies the output correctness after formatting
FORMATTING OPTIONS:
--call-parentheses <CALL_PARENTHESES>
Specify whether to apply parentheses on function calls with single string or table arg
[possible values: Always, NoSingleString, NoSingleTable, None, Input]
--collapse-simple-statement <COLLAPSE_SIMPLE_STATEMENT>
Specify whether to collapse simple statements [possible values: Never, FunctionOnly,
ConditionalOnly, Always]
--column-width <COLUMN_WIDTH>
The column width to use to attempt to wrap lines
--indent-type <INDENT_TYPE>
The type of indents to use [possible values: Tabs, Spaces]
--indent-width <INDENT_WIDTH>
The width of a single indentation level
--line-endings <LINE_ENDINGS>
The type of line endings to use [possible values: Unix, Windows]
--quote-style <QUOTE_STYLE>
The style of quotes to use in string literals [possible values: AutoPreferDouble,
AutoPreferSingle, ForceDouble, ForceSingle]
--sort-requires
Enable requires sortingThen at the end it says: =========================== short test summary info ============================
FAILED megalinter/tests/test_megalinter/linters/lua_stylua_test.py::lua_stylua_test::test_get_linter_help
===== 1 failed, 601 passed, 332 skipped, 52 warnings in 610.01s (0:10:10) ======
Pytest exited 1
Error(s) found by Pytest
Error: Process completed with exit code 1.Not sure what can be causing that. |
|
I think it might be just bad luck because of parallel tetibg updating the same file, i strarted it again :) |
Why don't we just add https://github.com/pytest-dev/pytest-rerunfailures?tab=readme-ov-file, like what https://hatch.pypa.io/latest/tutorials/testing/overview/#retry-failed-tests does (that we use). It might only help us a bit. |
|
@echoix that would save us time indeed :) |
|
@echoix @nvuillam I made a branch from |
I wasn't implying that you needed to do it at all, or that it was needed for this PR, just mentioning the solution when thinking about it, as it's been a recurring pain in the last months. I read the diff of your branch, and it's exactly what I had in mind and would have done myself if I was on a full computer with time for that. |
|
So... is everything done for this PR now? |
It was more like I did it on my own, I knew you didn't mean to say to add it. But in case you want to add it, I can make the PR or you can copy what I did. Of course it can be improve since I don't know where the point of failures might be or if there is some failures that don't need be retried it can use |
|
@echoix I think that's all for this PR. I will open an issue in Stylua's repo out of curiosity to see why might be failing with Docker and not with Cargo (maybe because of the distroless-cc, but uses cargo to build it too). If it can be fixed, it can be changed to use |
The part where we use alpine is important. It doesn't use glibc (by default) (it uses musl), and it is often expected to use static linking to have binaries that can just be copied and just run. I suspect that it could probably be copied from the docker image to any glibc-based image (almost any other distribution) |
Go ahead with a PR. We might just try it and find what we need to adjust more. But it's probably better than nothing, and waiting 30+min for a rerun. |
Fixes #3971
Proposed Changes
styluato descriptorscspell.jsonto include stylua related wordsstylua,luacheckandselenetest casesReadiness Checklist
Author/Contributor
Reviewing Maintainer
breakingif this is a large fundamental changeautomation,bug,documentation,enhancement,infrastructure, orperformance