Allow configuration through .clang-format files #1759
Conversation
| @@ -111,7 +111,7 @@ String format(ProcessRunner runner, String input, File file) throws IOException, | |||
| args = tmpArgs; | |||
| } | |||
| final String[] processArgs = args.toArray(new String[args.size() + 1]); | |||
| processArgs[processArgs.length - 1] = "--assume-filename=" + file.getName(); | |||
| processArgs[processArgs.length - 1] = "--assume-filename=" + file.getAbsolutePath(); | |||
There was a problem hiding this comment.
Interesting, this is all it takes for clang to find the .clang-format file?
The only problem is that the content of the .clang-format files is not being captured (ideally by FileSignature). As currently implemented the up-to-date checking would break. What do you think about clangFormat().dotfile('.clang-fomat') as an API? Is there usually only one .clang-format file, or are there sometimes multiple?
There was a problem hiding this comment.
Thanks for the reply.
While newer versions of clang-format allow to specify file path for the dot file, I think older versions only support --style=file which tells clang-format to recursively look for such a dot file in parent directories (of targeted .c/.h/... file) and uses first one it can find.
I'll look into it and will try your API suggestion + FileSignature thingy 👍 .
There was a problem hiding this comment.
If clang is actually searching with a recursive search, and we're specifying by pointing to a dotfile, I think that's okay. Of course there is a chance of some misconfiguration, but it's a lot better than giving the user no chance to capture the dotfile state at all.
There was a problem hiding this comment.
So looking into this, I learned 2 things so far: 🤓
-
According to the ClangFormat docs,
--style=fileis actually the default, meaning CF will already pickup on present dotfiles in current version of PR if you do not provide a.style(...).- I tried this locally with CF 14,13,12,11 on Ubuntu. My PR uses the dotfile without adding
.style('file')(still need to trigger it by modifying the .c file though)
- I tried this locally with CF 14,13,12,11 on Ubuntu. My PR uses the dotfile without adding
-
As of clang 14, the option also supports providing a path to a specific dot file
-style=file:<format_file_path>
That also means that for the devs who are currently using spotless without .style and have a .clang-format file in the directory (or a parent directory) of the file being formatted, this PR will cause the dotfile to be picked up.
Maybe that's fine but it's an impact you should be aware of.
Two remaining questions for you:
- Should I still look into detecting the dotfile changes? I can try if you want but not sure how long that will take me.
- If we wanna support clang14's format_file_path, should Spotless perform a version check, let older clang versions complain/error, or catch and wrap the older clang error?
- Current state of PR, providing
.style('file:/path/to/dotfile')on older CF results in :stderr: Invalid value for -style
- Current state of PR, providing
There was a problem hiding this comment.
If this
has a fieldFileSignature dotFile then it will detect dotfile changes. Right now, without this PR, I believe it will not detect dotfile changes becuase it doesn't know where the files are, so it can't find them even if they are there.
In terms of what to merge/implement, the most important thing is that it works for you. If you need to bump the required clang, we can bump it, and if someone needs support for older versions, they can add it.
Seems to me like a good compromise is if (dotFile == null) { currentBehaivorBeforePR } else { clang14 format_file_path }.
|
No rush, but I expect to make a release this week. Just FYI in case you are eager to have this in a published release. |
|
I'm currently very low on personal time 😅 so please don't wait for me. I quickly tried adding a FileSignature locally, updating it with the .clang-format hash, but changes were still not picked up. |
| private void resolveDotFile(File targetFile) throws IOException | ||
| { | ||
| // The dot file is irrelevant if a specific style other than "file" is supplied. | ||
| if (style != null && !style.equals("file")) { | ||
| return; | ||
| } | ||
|
|
||
| File directory = targetFile.getParentFile(); | ||
| Optional<File> dotFile = Optional.empty(); | ||
| while (dotFile.isEmpty() && readableDirectory(directory)) { | ||
| dotFile = Arrays.stream(directory.listFiles()).filter(file -> file.getName().equals(DOT_FILE_NAME)).findAny(); | ||
| directory = directory.getParentFile(); | ||
| } | ||
|
|
||
| System.out.println("dotFile: " + dotFile); | ||
| // Every target file can have a different .clang-format file (in theory). | ||
| // Keep track of the ones we've covered and build the sig as we go. | ||
| if (dotFile.isPresent() && !dotFiles.contains(dotFile.get())) { | ||
| dotFiles.add(dotFile.get()); | ||
| dotFileSig = FileSignature.signAsSet(dotFiles); | ||
| } | ||
| System.out.println("Signature" + dotFileSig); | ||
| } |
There was a problem hiding this comment.
This does not seem sufficient to detect .clang-format changes
nedtwigg
added
pr-archive
Please DO NOT FORCE PUSH. Don't worry about messy history, it's easier to do code review if we can tell what happened after the review, and force pushing breaks that.
Please make sure that your PR allows edits from maintainers. Sometimes its faster for us to just fix something than it is to describe how to fix it.
After creating the PR, please add a commit that adds a bullet-point under the
[Unreleased]section of CHANGES.md, plugin-gradle/CHANGES.md, and plugin-maven/CHANGES.md which includes:If your change only affects a build plugin, and not the lib, then you only need to update the
plugin-foo/CHANGES.mdfor that plugin.If your change affects lib in an end-user-visible way (fixing a bug, updating a version) then you need to update
CHANGES.mdfor both the lib and all build plugins. Users of a build plugin shouldn't have to refer to lib to see changes that affect them.This makes it easier for the maintainers to quickly release your changes :)