Closed nim65s closed 2 years ago
Hi @nim65s
Is this exclusively for this repo or is it meant to be used by projects that use jrl-cmakemodules?
This is only for this repo.
This PR doesn't change anything for projects that use jrl-cmakemodules, except if people run the same tools there, they won't complain anymore about issues in the submodule :)
Ok, thanks for claryfing.
I'm not sure which step is responsible for javascript formatting but can you filter out modifications to the doxygen/MathJax folder? Those files are taken from a minified MathJax version and it doesn't make sense to undo that.
I've ignored javascript files. ignoring doxygen/MathJax folder is another valid option, but I read your message too quickly to notice it. In the current case, this change nothing, but it can still be done.
I think that's ok, the only javascript files in the repo are from MathJax anyway :)
Have you looked at how a big a change we would get if we add a cmake-format and/or cmake-lint hook?
I didn't knew those tools. They clearly implement features which would be great for us. But I'm unsure about the project status: https://pypi.org/project/cmakelang/ says alpha, and the homepage is a broken link: https://github.com/cheshirekow/cmakelang
Installing it anyway allow to format the code, but the pre-commit hook fail even after a cmake-format
with a not-so-clear:
WARNING __main__.py:530: While processing pkg-config.pc.cmake
ERROR __main__.py:638: Unexpected UNQUOTED_LITERAL token at 1:0
Traceback (most recent call last):
File "…/cmakelang/format/__main__.py", line 633, in main
return inner_main()
File "…/cmakelang/format/__main__.py", line 616, in inner_main
onefile_main(infile_path, args, argparse_dict)
File "…/cmakelang/format/__main__.py", line 526, in onefile_main
outtext, reflow_valid = process_file(cfg, intext, args.dump)
File "…/cmakelang/format/__main__.py", line 158, in process_file
parse_tree = parse.parse(tokens, ctx)
File "…/cmakelang/parse/__init__.py", line 68, in parse
return BodyNode.consume(ctx, tokens)
File "…/cmakelang/parse/body_nodes.py", line 75, in consume
raise InternalError(
cmakelang.common.InternalError
Ok, it's obviously that file whch is a .pc but with a .cmake extension… And the homepage is https://github.com/cheshirekow/cmake_format
Here is a sub-PR for cmake-format: https://github.com/nim65s/jrl-cmakemodules/pull/1
What do you think ?
Here is a sub-PR for cmake-format: nim65s#1
What do you think ?
It looks reasonable and it nicely unifies the style across our CMake scripts but it touches file that were taken from older more current CMake versions, from a cursory look:
❯ ag OSI-approved
cython/python/FindPython.cmake
1:# Distributed under the OSI-approved BSD 3-Clause License. See accompanying
cython/python/FindPython/Support.cmake
1:# Distributed under the OSI-approved BSD 3-Clause License. See accompanying
cython/python/FindPython2.cmake
1:# Distributed under the OSI-approved BSD 3-Clause License. See accompanying
cython/python/FindPython3.cmake
1:# Distributed under the OSI-approved BSD 3-Clause License. See accompanying
boost/FindBoost.cmake
1:# Distributed under the OSI-approved BSD 3-Clause License. See accompanying
python/FindPythonInterp.cmake
1:# Distributed under the OSI-approved BSD 3-Clause License. See accompanying
python/FindPythonLibs.cmake
1:# Distributed under the OSI-approved BSD 3-Clause License. See accompanying
GNUInstallDirs.cmake
1:# Distributed under the OSI-approved BSD 3-Clause License. See accompanying
On a side-note, it would also be nice to add information about running/installing the hooks locally
Is your list is exhaustive ? If so, I can add those files to the exclude filter.
About running/installing the hooks, do you think a link to https://pre-commit.com/ would do ?
Otherwise:
# install pre-commit:
python -m pip install pre-commit
# run hooks on all files:
pre-commit run -a
# run automatically the hooks on the added / modified files, when you try to commit:
pre-commit install
Where should we put that, and which version (link or example) ?
PS: ag
? did you give https://github.com/BurntSushi/ripgrep a try ? :)
Is your list is exhaustive ?
I just went based on the license header but I don't remember we added others
Where should we put that, and which version (link or example) ?
In the README and/or the developper docs and/or the PR template (that does not exist)
I think the link and the example are both helpful, the example gives the main commands we'll need and the link allows to take it further
PS: ag ? did you give BurntSushi/ripgrep a try ? :)
I will. Thanks :-)
In the PR template, it is not necessary: https://pre-commit.ci/ (which is from the same author as https://pre-commit.com/) will automatically make the modifications for us :)
I'll update the exclude filter, and add the link and example in the README
I'll update the exclude filter, and add the link and example in the README
Great! Once this is done, feel free to merge :)
HI,
Coupled with https://pre-commit.ci/, this should allow us get and enforce some standards on format / lints.
The
.git-blame-ignore-revs
file can hide huge automatic changes on github blames, and on local git clients withgit config blame.ignoreRevsFile .git-blame-ignore-revs
For now, the chosen tools are:
but we can discuss about it