Ability To Turn Off Formatting For Subdirectory #873
Conversation
samdeane
requested review from
ahoppen,
allevato,
bnbarham and
shawnhyam
as code owners
There was a problem hiding this comment.
Thanks for looking into this! I think it's a useful feature to have. Some high-level comments:
-
I think the name of the file should be
.swift-format-ignore, which aligns nicely with the name used in comments that the formatter supports, and also similar tools like.gitignoreand.prettierignore. -
Lowering this down into the
SwiftFormatterandSwiftLinterAPIs feels like the wrong approach, because it means the tool is still doing all the work of iterating over and parsing all the files that are going to be ignored anyway.
For the second point, raising this check into the frontend seems more appropriate. Maybe FileIterator should be responsible for looking for the .swift-format-ignore file in the directory or its parents, and if one is found, it doesn't recurse into the directory further? That may end up being simpler to implement (you don't have to touch the configuration at all).
It means that somebody using the SwiftFormatter/SwiftLinter APIs directly wouldn't get that logic, but I think that's fine; those APIs are meant to be used by clients who want to format something directly and supplying a configuration in memory, and they're not interested in the search logic used by the frontend.
Yes, makes sense 👍 |
I think I was fixated on the idea that the ignoring mode needed to be configurable from within a I'll try your plan instead. |
|
Ok, I got rid of the setting and moved the check into FileIterator's directory handling. It works well for input directories, and for subdirectories reached via recursion, but doesn't work for input files (e.g: in the case where individual file paths are specified on the command line directly). For input files, we need to perform a check for |
That makes the most sense to me. |
|
Ok, 'tis a little closer now. One question is what it should do if The README still says that the filesystem won't be searched; which is true for The path of least resistance is to clarify the README, but that depends whether that's the behaviour we want. Alternatively I'll need to add a flag to FileIterator to indicate whether or not it should check for the ignore file. Which is perfectly doable, of course. |
The implication of that statement is that the filesystem won't be searched for the configuration (i.e., If we want a way to ignore the ignore file, we could add something like a |
Yeah, I was thinking the same. I'll reword the docs. |
| repeat { | ||
| containingDirectory.deleteLastPathComponent() | ||
| let candidateFile = containingDirectory.appendingPathComponent(FileIterator.ignoreFileName) | ||
| if FileManager.default.isReadableFile(atPath: candidateFile.path) { |
There was a problem hiding this comment.
More than just checking for the presence of the file, I think we go the extra step of validating that it only contains * so that users aren't surprised later when we add more functionality and it subtly changes the behavior based on those contents. If it contains anything else, we should throw an error saying it's the only supported form (and turn that into a nice human-readable error message when we exit).
There was a problem hiding this comment.
If we want to upgrade to gitignore like sytanx in the future, should we make the default value **/*?
There was a problem hiding this comment.
AFAIK, * is sufficient because according to the documentation:
- "An asterisk '
*' matches anything except a slash." - "If there is a separator at the beginning or middle (or both) of the pattern, [...]. Otherwise the pattern may also match at any level below the
.gitignorelevel." - "If there is a separator at the end of the pattern then the pattern will only match directories, otherwise the pattern can match both files and directories."
There was a problem hiding this comment.
Sorry, got distracted, but am looking at this now.
One complication is that FileIterator doesn't throw, and instead relies on the caller to generate diagnostics for non-existent paths.
In order to report invalid ignore files inside FileIterator we'd either need to be able to:
- throw an error from it
- be able to emit diagnostics directly from it
- return the path of the invalid ignore file (or a special pseudo-path), as if they were to be processed, then spot that path in the front end and emit an "invalid ignore file" diagnostic
I've gone for the third option, but it feels like a bit of a hack.
Draft implementation of #870.
This implementation adds a check for a
.swift-format-ignorefile, in the same places that a.swift-formatfile could be.If present, this file potentially contains gitignore-style rules describing which files to ignore.
Currently the only rule supported is
*, to ignore everything in the containing directory and any subdirectories. Later, full pattern-matching rules could be implemented, if desired. For now, we simply check that the content of the file is*, to ease any future migration problems.The presence of
.swift-format-ignoreis checked first at any given directory level. Even if a.swift-formatfile also exists at the same level, the ignore file takes precedence.Motivation
The use case which motivated this change is a scenario where my workspace has a
.swift-formatfile, and I've turned on format-on-save, but I also have submodules containing third-party code.If I make a small edit to a file in a submodule, it is formatted using the global workspace settings. This generates a large diff unrelated to my edit, as the submodule is hand-formatted using different rules.
I want to be able to disable formatting for specific submodules on my local machine, whilst retaining formatting for the rest of the workspace.