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

Gradually deprecate running swift-format without input paths; require '-' for stdin in the future #914

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

TTOzzi
Copy link
Contributor

@TTOzzi TTOzzi commented Jan 11, 2025

As discussed in #871 and in this forum thread Default behavior of “swift format”, waiting for stdin when only the swift format command is entered causes significant confusion for new users.
Although there were suggestions to provide a default behavior for convenience, I agree that it is not appropriate for a destructive change to take effect immediately.

However, at the very least, some guidance is needed to prevent the command from being perceived as hanging, so I've improved usability to output additional guidance when the user enters the swift format command alone.(same in swift format lint).

@TTOzzi TTOzzi force-pushed the more-friendly-commands branch 4 times, most recently from d4c9aa0 to 6356aca Compare January 12, 2025 09:40
@allevato
Copy link
Member

This would remove the ability to format/lint from stdin without also passing --assume-filename, which could be a breaking change in some workflows.

Before breaking any of the current usage patterns, at a minimum I think we need to start allowing - to be specified as a path that represents stdin and give users time to migrate to that. If someone uses swift-format (lint) without any path arguments, we could emit a warning that says something to the effect of running swift-format with no input paths is deprecated and will be removed in the future; specify '-' to use stdin but otherwise keep the current behavior for a release.

Then, after that release, we should revisit all of the default behaviors discussed in that thread holistically, so that we don't risk ending up in an intermediate stage if we were to make them arbitrarily or piecemeal.

@TTOzzi TTOzzi force-pushed the more-friendly-commands branch from 6356aca to d73a539 Compare January 13, 2025 15:03
@TTOzzi
Copy link
Contributor Author

TTOzzi commented Jan 13, 2025

This would remove the ability to format/lint from stdin without also passing --assume-filename, which could be a breaking change in some workflows.

Before breaking any of the current usage patterns, at a minimum I think we need to start allowing - to be specified as a path that represents stdin and give users time to migrate to that. If someone uses swift-format (lint) without any path arguments, we could emit a warning that says something to the effect of running swift-format with no input paths is deprecated and will be removed in the future; specify '-' to use stdin but otherwise keep the current behavior for a release.

Then, after that release, we should revisit all of the default behaviors discussed in that thread holistically, so that we don't risk ending up in an intermediate stage if we were to make them arbitrarily or piecemeal.

Oh, I missed that it's possible to format/lint without the --assume-filename option. I modified the implementation so that it works correctly when code is provided via stdin.
Regarding this approach, I used isatty to check if there is input from the terminal for the swift-format command. Since I couldn't test it on Windows, I kept the existing behavior. However, I believe this improvement for Linux and macOS will be helpful.
I would appreciate your thoughts on this approach.
(I think this is also a temporary measure until we revisit the discussions regarding the default behaviors.)

@TTOzzi TTOzzi force-pushed the more-friendly-commands branch from d73a539 to 08f3f57 Compare January 13, 2025 15:12
@ahoppen
Copy link
Member

ahoppen commented Jan 27, 2025

I prefer @allevato’s proposal to use - as an input file name to indicate that the input is read from stdin instead of checking using atty because:

  • There is wide precedence on other command line tools to indicate - as stdin
  • I don’t think that piping a source file to swift-format is widely used, so the requiring user’s to add - to their invocation isn’t unreasonable
  • The approach works on all platforms including Windows and is more explicit.

@DaveEwing
Copy link
Contributor

  • I don’t think that piping a source file to swift-format is widely used, so the requiring user’s to add - to their invocation isn’t unreasonable

IDEs probably do this. Xcode does. So they will have to update to support this change.

@ahoppen
Copy link
Member

ahoppen commented Jan 29, 2025

IDEs probably do this. Xcode does. So they will have to update to support this change.

It think it’s not unreasonable to ask these clients to update. In only expect there to be a handful.

@TTOzzi TTOzzi force-pushed the more-friendly-commands branch from 08f3f57 to 5afe05e Compare January 29, 2025 07:04
@TTOzzi TTOzzi changed the title Print a usage description when the user enters the swift format command without arguments Gradually deprecate running swift-format without input paths; require '-' for stdin in the future Jan 29, 2025
@TTOzzi
Copy link
Contributor Author

TTOzzi commented Jan 29, 2025

Thank you all for taking the time to share your thoughts 🙇
I have incorporated your feedback.

@TTOzzi TTOzzi force-pushed the more-friendly-commands branch 3 times, most recently from d443f45 to a2a0445 Compare January 29, 2025 07:56
@@ -108,7 +108,7 @@ struct LintFormatOptions: ParsableArguments {
var experimentalFeatures: [String] = []

/// The list of paths to Swift source files that should be formatted or linted.
@Argument(help: "Zero or more input filenames.")
@Argument(help: "Zero or more input filenames. If no paths are provided, stdin input will be used (use `-`).")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Argument(help: "Zero or more input filenames. If no paths are provided, stdin input will be used (use `-`).")
@Argument(help: "Zero or more input filenames. Use `-` for stdin.")

No reason to explicitly document the "no paths" behavior if we're planning to deprecate it in the future; let's just document the happy path.

Comment on lines 105 to 113
Running swift-format without input paths is deprecated and will be removed in the future.
Please specify one of the following:
- Use - to read from stdin (e.g., cat MyFile.swift | swift-format -)
- Provide a directory path (e.g., swift format --recursive MyDirectory/)
- Provide a specific Swift source file (e.g., swift format MyFile.swift)
For more information, use the --help option.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Running swift-format without input paths is deprecated and will be removed in the future.
Please specify one of the following:
- Use - to read from stdin (e.g., cat MyFile.swift | swift-format -)
- Provide a directory path (e.g., swift format --recursive MyDirectory/)
- Provide a specific Swift source file (e.g., swift format MyFile.swift)
For more information, use the --help option.
Running swift-format without input paths is deprecated and will be removed in the future.
Please update your invocation to do either of the following:
- Pass `-` to read from stdin (e.g., `cat MyFile.swift | swift-format -`).
- Pass one or more paths to Swift source files or directories containing
Swift source files. When passing directories, make sure to include the
`--recursive` flag.
For more information, use the `--help` option.

Just a little editorial cleanup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback!

@TTOzzi TTOzzi force-pushed the more-friendly-commands branch from a2a0445 to 0f1b558 Compare January 29, 2025 14:11
Copy link
Member

@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 for working on this! I know it's a lot smaller in scope than what you were originally looking to do, but staging this in gently will be important for our users.

@TTOzzi TTOzzi force-pushed the more-friendly-commands branch from 0f1b558 to 3f041ed Compare January 29, 2025 14:21
@allevato allevato merged commit f9b10f2 into swiftlang:main Jan 29, 2025
19 checks passed
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.

4 participants