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 |
README.md
Outdated
| Note that the contents of any `.swift-format-ignore` are ignored; the | ||
| file can be empty. |
There was a problem hiding this comment.
Thinking about this further, we may eventually want to allow users to specify patterns here like other tools' ignore files. So we should make sure that the behavior that we support today would upgrade cleanly to something more elaborate in the future. If we allow the file to be empty today and then decide that we want to allow patterns, that becomes an awkward path because an empty file ought to mean "ignore nothing" in that world.
The most obvious thing that comes to mind is for us to currently require that the contents of the .swift-format-ignore file are exactly * (modulo whitespace). Then anyone who wants to ignore a directory could put that in their file and it would continue to work later if we start expanding it to more complex patterns. When we find the file, we could open it and error out if the file contains anything other than *, telling users that that's the only supported option today.
So I would remove this paragraph and change the one you added further up to read something like this instead:
If it finds a file named
.swift-format-ignore, its contents will determine which files in that directory should be ignored by swift-format. Currently, the only supported option is*(ignores all files).
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."
Draft implementation of #870.
This implementation adds theskipAllflag toConfiguration.The user can set the flag manually, but we also search for a file called.no-swift-formatin the same places as we look for.swift-format. If we it, we return a Configuration withskipAllset.Whilst formatting, ifskipAllis set, we output the source file unmodified.Whilst linting, ifskipAllis set, we perform no checks and output no diagnostics.This implementation adds a check for a
.swift-format-ignorefile, in the same places that a.swift-formatfile could be.The presence of this file skips processing of any files in the containing directory, including those in subdirectories.
Configuration File Precedence
Note that the presence of
.swift-format-ignoreis checked first at any given directory level, and so it overrides any.swift-formatfile at the same level.The logic behind this choice is that allows
.swift-format-ignoreto be added to.gitignore, and to be created locally, even if a subdirectory already contains a.swift-formatfile that has been committed.Other choices could be made here, and there are pros & cons either way.
Motivation
My specific use case (which motivated this change) was a scenario where a gitsubmodule had a
.swift-formatfile at its root, but automated formatting was not yet in use, and the bulk of the module's code was not formatted according to the.swift-formatsettings.In this scenario I have a
.swift-formatfile at the root of my larger workspace, and have turned on format-on-save globally for the whole workspace.If I made a small edit to a file in the submodule, it was being formatted using the submodule's
.swift-formatfile -- which was a problem as it then generated a large diff unrelated to my edit.I wanted to disable this formatting on my local machine, in the submodule only, but leave the
.swift-formatfile in place for both my workspace and the submodule.