Skip to content

Fix-Its should be phrased as imperative actions rather than questions #67510

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

Closed
tristanlabelle opened this issue Jul 25, 2023 · 9 comments · Fixed by #67909
Closed

Fix-Its should be phrased as imperative actions rather than questions #67510

tristanlabelle opened this issue Jul 25, 2023 · 9 comments · Fixed by #67909
Assignees
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler itself conformances Feature → protocol: protocol conformances diagnostics QoI Bug: Diagnostics Quality of Implementation fix-its Feature: diagnostic fix-its good first issue Good for newcomers swift 5.9 type checker Area → compiler: Semantic analysis

Comments

@tristanlabelle
Copy link
Contributor

Swift phrases quick fixes as questions:

image

This is inconsistent with other extensions and code actions, which are phrased as imperative verbs:

image image image image
@ahoppen
Copy link
Member

ahoppen commented Jul 25, 2023

Tracked in Apple’s issue tracker as rdar://112830935

@ahoppen ahoppen changed the title Quick fixes should be phrased as imperative actions rather than questions Fix-Its should be phrased as imperative actions rather than questions Jul 25, 2023
@ahoppen
Copy link
Member

ahoppen commented Jul 25, 2023

I have been moving in the same direction of imperative Fix-It messages in new parser (https://github.com/apple/swift-syntax/blob/1a594a6296de3964756835bd57599f9c28c40229/Sources/SwiftParserDiagnostics/ParserDiagnosticMessages.swift#L582-L629) so I support the direction.

All these Fix-It messages are coming from the compiler, so I’m moving the issue to that repo. But just to set expectations, I don’t expect all the compiler Fix-It messages to get updated in the near future.

@ahoppen ahoppen transferred this issue from swiftlang/sourcekit-lsp Jul 25, 2023
@tristanlabelle
Copy link
Contributor Author

Sounds good, thank you! Other messages here start with a capital letter but sourcekit-lsp could make that conversion if swift-syntax has other clients with other expectations.

@hborla hborla added the good first issue Good for newcomers label Aug 1, 2023
@hborla hborla self-assigned this Aug 1, 2023
@hborla
Copy link
Member

hborla commented Aug 1, 2023

@nishithshah2211's interested in taking a look at this!

@XaurDesu
Copy link

Should we make a list of messages worded like this? I could maybe try and rewrite a few of them

@XaurDesu
Copy link

Actually now that i think about it, where are this messages defined, anyways?

@xedin
Copy link
Contributor

xedin commented Aug 13, 2023

All of the diagnostic messages are located in https://github.com/apple/swift/tree/main/include/swift/AST in various Diagnostics*.def files split into one file per compiler phase e.g. Parser, Sema, SIL as well as Common. There is an easy way to find out identifier of the diagnostic: add -Xfrontend -debug-diagnostic-names flag in your compiler invocation and use the identifier produced in braces to grep in the def files.

nishithshah2211 pushed a commit to nishithshah2211/swift that referenced this issue Aug 14, 2023
…se fixits

This commit changes fixit messages from a question/suggestion to an
imperative message for protocol conformances and switch-case. Addresses
swiftlang#67510.
nishithshah2211 pushed a commit to nishithshah2211/swift that referenced this issue Aug 14, 2023
…se fixits

This commit changes fixit messages from a question/suggestion to an
imperative message for protocol conformances and switch-case. Addresses
swiftlang#67510.
@vickipetrova
Copy link

Is there any work remaining on this issue -- I can see it's still open but there's a merged PR? I'm just looking for a simple first issue to tackle :)

@tristanlabelle
Copy link
Contributor Author

You're welcome to review other scenarios but I just verified that the specific bug I was reporting here was fixed, although now there are two different-named actions that will do the same.

image

@AnthonyLatsis AnthonyLatsis added compiler The Swift compiler itself diagnostics QoI Bug: Diagnostics Quality of Implementation type checker Area → compiler: Semantic analysis conformances Feature → protocol: protocol conformances fix-its Feature: diagnostic fix-its swift 5.9 bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. labels Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler itself conformances Feature → protocol: protocol conformances diagnostics QoI Bug: Diagnostics Quality of Implementation fix-its Feature: diagnostic fix-its good first issue Good for newcomers swift 5.9 type checker Area → compiler: Semantic analysis
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants