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.

Sign up for GitHub

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

Jump to bottom

silence #file to #filePath warnings #133

Merged
merged 2 commits into from Jun 9, 2020
Merged

silence #file to #filePath warnings #133

merged 2 commits into from Jun 9, 2020

Conversation

weissi
Copy link
Member

@weissi weissi commented Jun 1, 2020 •
edited 

Motivation:

Swift 5.3 will keep `#file` == `#filePath` but starts warning if you
pass `#file` to an argument that's `#filePath` defaulted (XCTest).

Modification:

- Replace `#file` with `(#file)` to silence the warning. In Swift 5.3
  `#file` == `#filePath`.

Result:

No warnings

@weissi weissi requested a review from tomerd June 1, 2020 18:23
@ktoso
Copy link
Member

ktoso commented Jun 4, 2020

Formatting issues: https://ci.swiftserver.group/job/swift-log-swift50-prb/162/console (whitespace)

PR LGTM, ok to keep compatibility and no surprising changes I guess.
In the long run honestly path is what we'll want but we need to discuss and get #module etc... Will make a separate ticket about it though.

@weissi
Copy link
Member Author

weissi commented Jun 4, 2020

I'm deliberately not fixing the formatting right now because I think this is blocked on https://bugs.swift.org/browse/SR-12934

@weissi weissi force-pushed the jw-filepath branch 2 times, most recently from 68dc009 to 2fc1809 Compare June 4, 2020 19:46
@weissi weissi mentioned this pull request Jun 4, 2020
Closed
@weissi weissi changed the title hand #filePath to LogHandlers silence #file to #filePath warnings Jun 4, 2020
@weissi weissi requested a review from ktoso June 4, 2020 19:58
@weissi
Copy link
Member Author

weissi commented Jun 4, 2020

@ktoso PR totally different now. Swift 5.3 actually keeps the behaviour, we just need to silence the XCTest warnings for now.

ktoso
ktoso reviewed Jun 8, 2020
View reviewed changes
.swiftformat
@@ -11,3 +11,5 @@
--ifdef no-indent

# rules

--disable redundantParens # https://github.com/nicklockwood/SwiftFormat/issues/638
Copy link
Member

Choose a reason for hiding this comment

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

👍

ktoso
ktoso reviewed Jun 8, 2020
View reviewed changes
Tests/LoggingTests/TestLogger.swift Outdated
@@ -292,3 +300,14 @@ internal struct TestLibrary {
}
}
}

#if compiler(>=5.3)
internal func fullFilePath(_ filePath: StaticString = #filePath) -> StaticString {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this one in tests?
If #file == #filePath but just the warning around = #file sounds like there should be no difference on how we treat this in tests and the lib itself?

ktoso
ktoso reviewed Jun 8, 2020
View reviewed changes
Sources/Logging/Logging.swift Outdated Show resolved Hide resolved
ktoso
ktoso requested changes Jun 8, 2020
View reviewed changes
Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Thanks a lot for digging into and sanity checking all this.

Not sure all the comments / usage are consistent now?

@weissi weissi force-pushed the jw-filepath branch 2 times, most recently from 0558512 to a12d094 Compare June 8, 2020 09:57
@weissi
silence #file to #filePath warnings
2baa1c1 
Motivation:

Swift 5.3 will keep `#file` == `#filePath` but starts warning if you
pass `#file` to an argument that's `#filePath` defaulted (XCTest).

Modification:

- Replace `#file` with `(#file)` to silence the warning. In Swift 5.3
  `#file` == `#filePath`.

Result:

No warnings
@ktoso
Copy link
Member

ktoso commented Jun 8, 2020 •
edited

// was meant for: #135

Status update — I think this likely will be fine but I’ve ran out of time in the day to get it to completion in my project to really see it work out end to end.

It makes the label pretty useless but I guess we’ll agree that’s what it is. I’ll post a review tomorrow

@weissi
Copy link
Member Author

weissi commented Jun 8, 2020 •
edited by ktoso

// was meant for: #135

Status update — I think this likely will be fine but I’ve ran out of time in the day to get it to completion in my project to really see it work out end to end.

It makes the label pretty useless but I guess we’ll agree that’s what it is. I’ll post a review tomorrow

I thought about label more and I actually now think it's useful. It's basically orthogonal to source.

Let's consider a whole system and let's assume the system offers 2 services, let's call them service A and service B. Services A & B both use a multitude of sub-subsystems. Then everything that belongs to Service A gets label=MyApp.ServiceA and B gets label=MyApp.ServiceB. If now both A & B use say AsyncHTTPClient, then the label suddenly becomes useful.

  • source=AHC gives us AHC calls from both A & B which might be annoying if I'm a developer working only on A
  • source=AHC & label=MyApp.ServiceA however gives me exactly what I want.

@weissi weissi requested a review from ktoso June 8, 2020 16:14
@weissi
Copy link
Member Author

weissi commented Jun 8, 2020

@ktoso I think we're discussing this on the wrong issue actually :P

@ktoso
Copy link
Member

ktoso commented Jun 9, 2020

@ktoso I think we're discussing this on the wrong issue actually :P

Whoops, you're right 😱

Moving to #135

ktoso
ktoso approved these changes Jun 9, 2020
View reviewed changes
Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ktoso ktoso merged commit ee8b3c5 into apple:master Jun 9, 2020
@ktoso ktoso added this to the 1.3.0 milestone Jun 9, 2020
@ktoso ktoso mentioned this pull request Jun 9, 2020
Merged
@tanner0101 tanner0101 mentioned this pull request Jun 23, 2020
Merged
golang4gg pushed a commit to golang4gg/swift-log that referenced this pull request Jul 4, 2020
@weissi
silence #file to #filePath warnings (apple#133)
9f4ec00 
Motivation:

Swift 5.3 will keep `#file` == `#filePath` but starts warning if you
pass `#file` to an argument that's `#filePath` defaulted (XCTest).

Modification:

- Replace `#file` with `(#file)` to silence the warning. In Swift 5.3
  `#file` == `#filePath`.

Result:

No warnings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ktoso ktoso ktoso approved these changes

@tomerd tomerd tomerd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@weissi @ktoso @tomerd