Add dump-effective-configuration subcommand #923

andre-richter · 2025-02-06T14:24:08Z

closes #667

Implement the subcommand dump-effective-configuration, which dumps the configuration that would be used if swift-format was executed from the current working directory (cwd), incorporating configuration files found in the cwd or its parents, or input from the --configuration option.

This helps when composing a configuration or with configuration debugging/verification activities.

allevato

A high-level suggestion before I review this further: Since this is so similar to the existing dump-configuration command, I think this would be expressed more simply as an optional flag for that command instead of a completely separate command. Something like dump-configuration --effective would work and would probably cut down on the implementation complexity here.

andre-richter · 2025-02-06T17:29:27Z

I thought about this initially, and wasn't happy that it would need to be explained that the --configuration option will only be valid in combination with the --effective flag, so I went with separate subcommand.

I'll give merging this into the dump-config subcommand a try and amend it to this PR.

andre-richter · 2025-02-06T19:07:01Z

@allevato implemented your change request.

ahoppen · 2025-02-06T21:48:49Z

I’m curious what the real-world use case for that options is. swift-format doesn’t have a complicated search algorithm to find the format files (it just walks up the directory structure) and it doesn’t merge files either. So, IIUC, all this effectively does is to find the first .swift-format file in the directory structure and print it. Am I missing something?

allevato · 2025-02-07T03:31:19Z

I’m curious what the real-world use case for that options is. swift-format doesn’t have a complicated search algorithm to find the format files (it just walks up the directory structure) and it doesn’t merge files either. So, IIUC, all this effectively does is to find the first .swift-format file in the directory structure and print it. Am I missing something?

I'm not sure of the PR author's motivation, but one thought I've had in the past is that if we ever make a breaking change to the configuration and bump the version number, it would be nice to have a way for users to explicitly update their configuration to the new version. One way to handle that would be an option like this: since older versions of the config would be translated to the new model in-memory during decoding, printing back that in-memory model afterwards would render it as the latest version.

andre-richter · 2025-02-07T07:48:53Z

Sure, let me elaborate a bit on the motivation:

Closing a feature gap with similar tools like clang-format. Or git config -l as mentioned by the author of Add command to show the current configuration #667. For both examples, the default behavior is actually what --effective would do here (I am not implying this is a better or worse default).
What @allevato said. To extend on this: In general, having a quick way to verify that the configuration you're entering into the tool is actually being processed by the tool as intended is beneficial. This typically would be used while you're in the process of composing or modifying a .swift-format file or a --configuration JSON-string.
Tools or scripts that wrap around swift-format likely pass the configuration to the tool using the --configure option. --effective would allow authors of such wrappers to easily add an option to print the final configuration that would be used by swift-format, so that users get an option to easily retrieve it for review or re-use.

ahoppen

Thanks for your explanation of the use case @andre-richter. The change makes sense to me. I have a few comments regarding the implementation inline.

Sources/SwiftFormat/API/Configuration+Dump.swift

ahoppen · 2025-02-10T21:49:04Z

Sources/swift-format/Subcommands/DumpConfiguration.swift

+      let frontend = DumpConfigurationFrontend(
+        configurationOptions: configurationOptions,
+        lintFormatOptions: lintFormatOptions
+      )
+      frontend.run()


I’m not a big fan of running a dummy frontend that stores the configuration used as a side effect. I’d prefer to just call into Frontend.configuration(fromPathOrString:orInfoerredFromSwiftFileAt:) instead.

You mean by turning Frontend.configuration static somehow? Asking because the current implementation uses instance properties such as diagnosticsEngine.

Adding a dummy fronted seemed the least intrusive change for existing code to me, but I’m happy to make adaptions if needed.

Yes, looks like it shouldn’t be too hard. It looks like ConfigurationLoader can be a local variable instead of a member and DiagnsoticsEngine can be passed in.

andre-richter · 2025-02-11T10:56:41Z

@ahoppen, there was also checkForUnrecognizedRules(), which also employs a DiagnosticsEngine. Since Frontend.configuration() was the only user, passing DiagnosticsEngine references around in arguments several levels deep would have made the code quite convoluted in my opinion.

Therefore, I decided to carve out the whole configuration resolution logic into its own helper struct that saves a DiagnosticsEngine reference . I believe this also enhanced the frontend design slightly because it leads to a clearer division of responsibilities.

closes swiftlang#667 Implement the subcommand `dump-effective-configuration`, which dumps the configuration that would be used if `swift-format` was executed from the current working directory (cwd), incorporating configuration files found in the cwd or its parents, or input from the `--configuration` option. This helps when composing a configuration or with configuration debugging/verification activities.

ahoppen

Thanks for addressing my comments @andre-richter, this seems a lot cleaner to me. I have a few more questions/comments inline.

Sources/swift-format/Subcommands/DumpConfiguration.swift

ahoppen · 2025-02-12T18:53:52Z

Sources/swift-format/Frontend/Frontend.swift

+    /// Loads formatter configuration files and chaches them in memory.
+    private var configurationLoader: ConfigurationLoader = ConfigurationLoader()


Actually, I don’t think this needs to be a member at all. A local variable in provide should be sufficient, right?

It stores the cache of previously loaded configurations. So it wouldn't work as a local variable (as in: stack-allocated inside provide, if that is what you meant?).

ahoppen · 2025-02-12T18:57:20Z

Sources/swift-format/Frontend/Frontend.swift

+  /// Provides formatter configurations for given `.swift` source files, configuration files or configuration strings.
+  struct ConfigurationProvider {


Just wondering: Would it be simpler if we just had a static method (or global function) like func configuration(forConfigPathOrString pathOrString: String?, orForSwiftFileAt swiftFileURL: URL?, diagnosticsEngine: DiagnosticsEngine) and then either make checkForUnrecognizedRules a nested function or make it take a DiagnosticsEngine as well? To me, having a type here implies that there is some state being stored, which isn’t actually the case.

To me, having a type here implies that there is some state being stored, which isn’t actually the case.

There is state being stored in the form of configurationLoader, which stores the cache for previously loaded configs.

Oh, I see. Fair enough then.

andre-richter requested review from ahoppen, allevato, bnbarham and shawnhyam as code owners February 6, 2025 14:24

allevato reviewed Feb 6, 2025

View reviewed changes

ahoppen reviewed Feb 10, 2025

View reviewed changes

andre-richter force-pushed the dump-effective-conf branch from 4ee4610 to 805bdec Compare February 12, 2025 07:59

andre-richter added 3 commits February 12, 2025 11:27

Merge into single subcommand

8172c06

Do not use dummy frontend for effective config printing

1e8c05e

andre-richter force-pushed the dump-effective-conf branch from 805bdec to 1e8c05e Compare February 12, 2025 10:28

ahoppen reviewed Feb 12, 2025

View reviewed changes

ahoppen approved these changes Feb 14, 2025

View reviewed changes

ahoppen enabled auto-merge February 14, 2025 00:17

auto-merge was automatically disabled February 14, 2025 16:34
Head branch was pushed to by a user without write access

Incorporate review comments; Allow API breakage

11d6de8

andre-richter force-pushed the dump-effective-conf branch from 9c16a60 to 11d6de8 Compare February 14, 2025 16:35

ahoppen enabled auto-merge February 14, 2025 16:39

ahoppen merged commit 26930a6 into swiftlang:main Feb 14, 2025
19 checks passed

andre-richter deleted the dump-effective-conf branch February 14, 2025 20:48

		/// Loads formatter configuration files and chaches them in memory.
		private var configurationLoader: ConfigurationLoader = ConfigurationLoader()

		/// Provides formatter configurations for given `.swift` source files, configuration files or configuration strings.
		struct ConfigurationProvider {

Add dump-effective-configuration subcommand #923

Add dump-effective-configuration subcommand #923

Uh oh!

Conversation

andre-richter commented Feb 6, 2025

Uh oh!

allevato left a comment

Choose a reason for hiding this comment

Uh oh!

andre-richter commented Feb 6, 2025

Uh oh!

andre-richter commented Feb 6, 2025

Uh oh!

ahoppen commented Feb 6, 2025

Uh oh!

allevato commented Feb 7, 2025

Uh oh!

andre-richter commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andre-richter commented Feb 11, 2025

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andre-richter commented Feb 7, 2025 •

edited

Loading