[ST-NNNN] Macro for getting the current source location#3312
Conversation
grynspan
added
workgroup: needs pitch
grynspan
force-pushed
the
jgrynspan/sourcelocation-macro
branch
from
29ab3c8 to
1d59663
Compare
| - **Naming the macro something different.** We considered alternatives such as | ||
| `#here` and `#currentSourceLocation`, but `#sourceLocation` seems the most | ||
| appropriate name for it. |
There was a problem hiding this comment.
In this alternative, I'd find it helpful to reference back to the earlier section of the proposal where you mentioned the need to fully-qualify the macro with a module selector, and say that is the primary reason why an alternate name might be desirable. Just to clarify that this is more than run-of-the-mill bike shedding, there's a usability consideration. (Not that I think we should use a different name, I just think the bullet could provide more context.)
| This macro, being underscored, is not formally supported, nor does it appear in | ||
| Swift Testing's documentation. It is also not sufficient to use something like | ||
| [`SourceLocation.init()`](https://developer.apple.com/documentation/testing/sourcelocation/init(fileid:filepath:line:column:)) | ||
| as it will capture the _wrong_ source location. Thus, test authors have no |
There was a problem hiding this comment.
You could mention SE-0422 here as the underlying reason why a macro makes this possible
| the module name when using this macro to avoid conflicting with the [`#sourceLocation(file:line:)`](https://docs.swift.org/swift-book/documentation/the-swift-programming-language/statements/#Line-Control-Statement) | ||
| statement built into the Swift language. | ||
|
|
||
| The existing `#_sourceLocation` macro will be marked deprecated and renamed, but |
There was a problem hiding this comment.
…will be marked deprecated and renamed, …
The existing one will be renamed? (To what?)
There was a problem hiding this comment.
Oh, renamed: "Testing::sourceLocation" in @available. That's fair, but I found the phrasing a bit confusing since it sounded like you planned to rename the old macro. Maybe avoid that by just saying it will be deprecated?
|
|
||
| This macro, being underscored, is not formally supported, nor does it appear in | ||
| Swift Testing's documentation. It is also not sufficient to use something like | ||
| [`SourceLocation.init()`](https://developer.apple.com/documentation/testing/sourcelocation/init(fileid:filepath:line:column:)) |
There was a problem hiding this comment.
It's not obvious to me why this init would have the wrong source location. It seems like it would be incumbent on the caller to pass in the correct arguments (as mentioned below) as the initialiser doesn't have any default values right?
There was a problem hiding this comment.
See here for the proposal which explains it. And I agree it's non-obvious to readers who aren't aware of this macro nuance. I'd suggest mentioning that proposal
| /// // ... | ||
| /// } | ||
| /// ``` | ||
| +@available(swift, deprecated: 100000.0, renamed: "Testing::sourceLocation") |
There was a problem hiding this comment.
I think we ought to deprecate it "for real" and not just use the 10000.0 "to-be-deprecated" marker. (That's what this is, right? Hope I didn't get that wrong.)
Do you think there's a compelling reason to not do that?
There was a problem hiding this comment.
We generally don't do formal deprecation in the same release where we introduce a new replacement API as it causes undue noise/churn for developers. Which is why we have API_TO_BE_DEPRECATED in the first place.
There was a problem hiding this comment.
Users wanting to maintain support for building using an older toolchain is a reasonable justification, thanks.
How about we clarify in the proposal that it will be deprecated using the special "to-be-deprecated" marker initially, but that later (after some settling period TBD), the intention is to lower the deprecated-in version down to a specific Swift version retroactively. That way, the proposal states the intent which is to deprecate it fully—it's just that we won't do so in the immediate term.
grynspan
added
workgroup: ready
3 participants
See here.