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.

Sign up for GitHub

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

Allow configuration through .clang-format files #1759

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Allow configuration through .clang-format files #1759

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
daa0ef4
Use full path in clang-format --assume-filename
Jul 15, 2023
b7cd491
Update documentation
Jul 16, 2023
34ff3c0
Add changes.md
Jul 16, 2023
e5d09fe
Add FileSignature field to ClangFormatStep State
Aug 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
* Bump default `ktlint` version to latest `0.49.1` -> `0.50.0`. ([#1741](https://github.com/diffplug/spotless/issues/1741))
* Dropped support for `ktlint 0.47.x` following our policy of supporting two breaking changes at a time.
* Dropped support for deprecated `useExperimental` parameter in favor of the `ktlint_experimental` property.
* Use full path for clang-format's `--assume-filename` parameter, allowing disovery of .clang-format files. ([#1759](https://github.com/diffplug/spotless/pull/1759))

## [2.39.0] - 2023-05-24
### Added
Expand Down
2 changes: 1 addition & 1 deletion lib/src/main/java/com/diffplug/spotless/cpp/ClangFormatStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍 .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@EmielMatthys EmielMatthys Jul 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So looking into this, I learned 2 things so far: 🤓

  1. According to the ClangFormat docs, --style=file is 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)

  2. 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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this

spotless/lib/src/main/java/com/diffplug/spotless/cpp/ClangFormatStep.java

Lines 89 to 96 in b6e30f8

static class State implements Serializable {
private static final long serialVersionUID = -1825662356883926318L;
// used for up-to-date checks and caching
final String version;
final @Nullable String style;
final transient ForeignExe exe;
// used for executing
private transient @Nullable List<String> args;
has a field FileSignature 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 }.

return runner.exec(input.getBytes(StandardCharsets.UTF_8), processArgs).assertExitZero(StandardCharsets.UTF_8);
}

Expand Down
2 changes: 2 additions & 0 deletions plugin-gradle/CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
### Added
* Add target option `targetExcludeIfContentContains` and `targetExcludeIfContentContainsRegex` to exclude files based on their text content. ([#1749](https://github.com/diffplug/spotless/pull/1749))
* Add an overload for `FormatExtension.addStep` which provides access to the `FormatExtension`'s `Provisioner`, enabling custom steps to make use of third-party dependencies.
* Add option to specify `file` as `clangFormat` style to use local `.clang-format` files. ([#1759](https://github.com/diffplug/spotless/pull/1759))

### Fixed
* Correctly support the syntax
```
Expand Down
4 changes: 3 additions & 1 deletion plugin-gradle/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1014,7 +1014,9 @@ spotless {

// can also specify a code style
clangFormat().style('LLVM') // or Google, Chromium, Mozilla, WebKit
// TODO: support arbitrary .clang-format

// you can also indicate to use a local .clang-format file
clangFormat().style('file')

// if clang-format is not on your path, you must specify its location manually
clangFormat().pathToExe('/usr/local/Cellar/clang-format/10.0.1/bin/clang-format')
Expand Down