Skip to content

Uppercase first letter of diagnostics #784

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

Merged
merged 1 commit into from
Nov 1, 2023

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Jul 25, 2023

sourcekitd returns diagnostics with the first letter lowercase. Xcode, for example, shows the messages with the first letter uppercases. I think that looks nicer and we should also uppercase the first letter in sourcekit-lsp.

CC @tristanlabelle I noticed this in swiftlang/swift#67510. While it doesn’t change the messages to be imperative, having them start with an uppercase letter maybe makes them stick out a little less.

@ahoppen ahoppen requested a review from benlangmuir as a code owner July 25, 2023 16:53
@ahoppen
Copy link
Member Author

ahoppen commented Jul 25, 2023

@swift-ci Please test

@benlangmuir
Copy link
Contributor

My concern here is that the first word in the diagnostic might be a keyword or identifier that shouldn't be modified. If this is preferred style, why not update the compiler diagnostics themselves?

@ahoppen ahoppen force-pushed the ahoppen/uppercase-diag-message branch from 3e3b101 to 7047bc3 Compare July 25, 2023 18:51
@ahoppen
Copy link
Member Author

ahoppen commented Jul 25, 2023

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Jul 27, 2023

I think on the command line we want to emit diagnostics that start with a lowercase letter to match clang. But in IDEs the errors show up more nicely if they start with an uppercase letter.

If the diagnostic starts with an identifier or keyword, that identifier/keyword should be wrapped in single quotes, which means that the quote would be the first character, so we wouldn’t uppercase the identifier or keyword.

@ahoppen ahoppen force-pushed the ahoppen/uppercase-diag-message branch from 7047bc3 to 188677f Compare July 27, 2023 22:28
@ahoppen
Copy link
Member Author

ahoppen commented Jul 27, 2023

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Jul 27, 2023

@swift-ci Please test Windows

1 similar comment
@ahoppen
Copy link
Member Author

ahoppen commented Jul 28, 2023

@swift-ci Please test Windows

@bnbarham
Copy link
Contributor

I think on the command line we want to emit diagnostics that start with a lowercase letter to match clang. But in IDEs the errors show up more nicely if they start with an uppercase letter.

Do you know if this is actually true? Might be worth bringing up the conversation. FWIW someone actually did the imperative switch: swiftlang/swift#67909 :)

@ahoppen ahoppen force-pushed the ahoppen/uppercase-diag-message branch from 188677f to d52df40 Compare September 18, 2023 19:49
@ahoppen
Copy link
Member Author

ahoppen commented Sep 18, 2023

@swift-ci Please test

@ahoppen ahoppen force-pushed the ahoppen/uppercase-diag-message branch from d52df40 to 0b911f6 Compare November 1, 2023 00:20
@ahoppen
Copy link
Member Author

ahoppen commented Nov 1, 2023

@swift-ci Please test

sourcekitd returns diagnostics with the first letter lowercase. Xcode, for example, shows the messages with the first letter uppercases. I think that looks nicer and we should also uppercase the first letter in sourcekit-lsp.
@ahoppen ahoppen force-pushed the ahoppen/uppercase-diag-message branch from 0b911f6 to d4f304e Compare November 1, 2023 02:28
@ahoppen
Copy link
Member Author

ahoppen commented Nov 1, 2023

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Nov 1, 2023

@swift-ci Please test Windows

@ahoppen ahoppen merged commit 30d1db4 into swiftlang:main Nov 1, 2023
@ahoppen ahoppen deleted the ahoppen/uppercase-diag-message branch November 1, 2023 04:46
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.

3 participants