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

Rename note to notification throughout the codebase #893

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

issamarabi
Copy link

Fixes #865

  • Replaced occurrences of note with notification.
  • Updated related comments and documentation.

Originated from feedback by @ahoppen regarding terminology confusion. This change aims to maintain clarity across the codebase.

Reference: Apple’s issue tracker rdar://116703667

Please review.

Copy link
Collaborator

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thanks @issamarabi. I have a few comments inline but otherwise it looks good to me.

Sources/LSPTestSupport/TestJSONRPCConnection.swift Outdated Show resolved Hide resolved
Sources/LSPTestSupport/TestJSONRPCConnection.swift Outdated Show resolved Hide resolved
Sources/LSPTestSupport/TestJSONRPCConnection.swift Outdated Show resolved Hide resolved
Sources/LSPTestSupport/TestJSONRPCConnection.swift Outdated Show resolved Hide resolved
Comment on lines 22 to 23
/// If this is from a note, the note's description should be passed as `fromNote`.
init?(fixits: SKDResponseArray, in snapshot: DocumentSnapshot, fromNote: String?) {
/// If this is from a notification, the notification's description should be passed as `fromNotification`.
init?(fixits: SKDResponseArray, in snapshot: DocumentSnapshot, fromNotification: String?) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is about diagnostics, which do indeed have notes, not notifications. Could you change it back?

Copy link
Author

Choose a reason for hiding this comment

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

I changed these back. Let me know if this works!

if let diag = CachedDiagnostic(diag, in: snapshot, useEducationalNoteAsCode: supportsCodeDescription) {
if let diag = CachedDiagnostic(diag, in: snapshot, useEducationalNotificationAsCode: supportsCodeDescription) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is about notes, not notifications, so useEducationalNoteAsCode is correct.

@@ -760,11 +760,11 @@ final class LocalSwiftTests: XCTestCase {
)
}
} else {
XCTFail("missing '?' note")
XCTFail("missing '?' notification")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is talking about diagnostics and should be note.

XCTFail("missing '!' note")
XCTFail("missing '!' notification")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, talking about diagnostics.

@ahoppen
Copy link
Collaborator

ahoppen commented Oct 13, 2023

Argh, looks like this conflicted quite heavily with #896. Could you rebase the PR on top of main.

Sorry for causing the conflicts.

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.

Rename note to notification throughout the codebase
2 participants