Navigation Menu

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.

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

Adopt ArgumentParser for the command-line tool #154

Merged
merged 5 commits into from Mar 10, 2020

Conversation

natecook1000
Copy link
Member

This adopts the new ArgumentParser library for parsing command-line arguments.

@allevato allevato self-assigned this Feb 27, 2020
Package.swift Outdated
@@ -23,9 +23,10 @@ let package = Package(
dependencies: [
.package(
url: "https://github.com/apple/swift-syntax",
.revision("swift-DEVELOPMENT-SNAPSHOT-2020-01-29-a")
.branch("master")

Choose a reason for hiding this comment

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

better to use a specific branch, tag or commit

Choose a reason for hiding this comment

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

such as swift-5.2-branch

Copy link
Collaborator

Choose a reason for hiding this comment

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

For the swift-syntax dependency, please revert this back to the development snapshot that we were previously using. (We plan to switch to master soon so that we can hook into Swift's CI and have it build using the toolchain sources to detect breakages if someone updates the syntax definitions in an apple/swift PR, but we need to do some other prep work for that first.)

Copy link
Collaborator

@allevato allevato left a comment

Choose a reason for hiding this comment

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

The new argument parser looks great! Really excited to start using it. A few questions/comments below.

Package.swift Outdated
@@ -23,9 +23,10 @@ let package = Package(
dependencies: [
.package(
url: "https://github.com/apple/swift-syntax",
.revision("swift-DEVELOPMENT-SNAPSHOT-2020-01-29-a")
.branch("master")
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the swift-syntax dependency, please revert this back to the development snapshot that we were previously using. (We plan to switch to master soon so that we can hook into Swift's CI and have it build using the toolchain sources to detect breakages if someone updates the syntax definitions in an apple/swift PR, but we need to do some other prep work for that first.)

throw CleanExit.message("0.0.1")
}

if inPlace && (mode == .format || paths.isEmpty) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like part of this got flipped; it should be mode != .format.

diagnosticEngine.diagnose(
Diagnostic.Message(.error, "Unable to read source for linting from \(assumingFileURL.path)."))
return 1
throw FormatError.readSource(path: assumingFileURL.path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In #144, @dylansturg refactored lintMain and formatMain a bit so that the error messages here were sent through the diagnostic engine (instead of being written directly to stderr)—this unified all possible error messages that the tool might emit after argument parsing. For example, we could add a flag later to replace the diagnostic consumer with one that serializes to JSON instead of printing them to stderr, and file I/O errors would be handled seamlessly.

Changing these to throw errors here would revert that back to having two different kinds of behavior for different kinds of errors. Do we need this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll definitely revert that — it looked like I was replicating the current usage, but of course coalescing errors to look forward at using other outputs makes tons of sense.

return Int32(
formatMain(
extension SwiftFormatCommand {
func run() throws {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since ArgumentParser has subcommand support, it would make sense to have format, lint, and dump-configuration be subcommands with format as the default subcommand. It looks like that would also improve the command line validation because options could be restricted to just the commands that they apply to. But we also have internal tooling that depends on the current command line interface, so we wouldn't want to do that breaking change here yet.

As a possible follow-up PR, does ArgumentParser's design let us easily create those subcommands and then have the old -m/--mode flag delegate to them as a migration path so that we could have both interfaces implemented simultaneously, and then once we have our other tooling no longer depending on the old interface, we could remove it from swift-format?

Copy link
Member Author

Choose a reason for hiding this comment

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

We should totally be able to have those parallel interfaces. I have an old version of swift-format migrated to a subcommand interface, so after this is ready to merge I'll update that and open another PR.

Copy link
Contributor

@dylansturg dylansturg left a comment

Choose a reason for hiding this comment

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

I notice that whenever there are diagnostics, there's always an extra line printed to stderr that just has "Error:". Presumably that would normally contain a message from the error that was thrown, which is intentionally empty in this case. Is there a way to have a nonzero exit status and not print that extra "Error"? The diagnostics are being printed, so I don't think there's a need to also print that there was an error.

@@ -76,7 +75,7 @@ func lintMain(
func formatMain(
configuration: Configuration, sourceFile: FileHandle, assumingFilename: String?, inPlace: Bool,
debugOptions: DebugOptions, diagnosticEngine: DiagnosticEngine
) -> Int {
) {
let formatter = SwiftFormatter(configuration: configuration, diagnosticEngine: diagnosticEngine)
Copy link
Contributor

Choose a reason for hiding this comment

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

I recently changed this to pass nil, so that DiagnosticEngine is only used for fatal messages when formatting, in #155. It looks like this is causing a merge conflict - can you update and resolve the conflict?

@natecook1000
Copy link
Member Author

@dylansturg:
Is there a way to have a nonzero exit status and not print that extra "Error"?

Good point. This looks like something we'll want to add to ArgumentParser.

@allevato
Copy link
Collaborator

allevato commented Mar 2, 2020

Good point. This looks like something we'll want to add to ArgumentParser.

Is this something you'd add in the near future or later? If it's soon we could just wait for it, but if it's later, I don't want to block the rest of this change on it. @dylansturg suggested offline that we could change the FormatError message to something like "Exiting due to previous errors." The extra message isn't ideal but at least it would be temporary, and you could tag the message with a TODO comment to replace it once we have a way to silently exit with a non-zero status.

@natecook1000
Copy link
Member Author

I'll add it today, so it will be in the 0.0.2 release. We can just leave out the Error: prefix if there's nothing to show afterward.

@natecook1000
Copy link
Member Author

Updated this to use the new ArgumentParser functionality that allows exiting with a failure code without adding to the stderr output.

Copy link
Collaborator

@allevato allevato left a comment

Choose a reason for hiding this comment

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

Thanks! Alright, let's get this merged. I'm excited to be able to be able to keep building on top of it.

@allevato allevato merged commit 6449476 into apple:master Mar 10, 2020
@natecook1000 natecook1000 deleted the argumentparser branch March 10, 2020 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants