-
Notifications
You must be signed in to change notification settings - Fork 126
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
Adding deprecated to @Available directive #851
Adding deprecated to @Available directive #851
Conversation
a80b723
to
c2d167f
Compare
d688517
to
8fff1c0
Compare
@mustiikhalil Could you please add the Steps to reproduce section to the PR template, including the expected behaviour and a test DocC catalog that uses the new deprecated keyword? |
Yes, i can do that later today. @sofiaromorales regarding the test bundle i thought it would be enough to include it within the current test bundle which can be found in AvailabilityBundle.docc since its just an arguement to the same directive |
552c4bf
to
86da846
Compare
02e741d
to
b5cc26e
Compare
@sofiaromorales sorry for the ping, but would love to know what's next to get this PR moving forward |
@@ -77,6 +74,7 @@ extension Metadata { | |||
case .iOS: return "iOS" | |||
case .watchOS: return "watchOS" | |||
case .tvOS: return "tvOS" | |||
case .any: return "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure this is how we want to display any
?
@mustiikhalil can you attach an screenshot on how does this looks on the rendered documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how this is meant to work but I don't think we'd want this to display *
here.
It's a bit odd that the wildcard specifies a version at all because that version may not align across the various platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably say that the correct way would be to list the deprecation on platform where the symbol is known to be available.
However, like I said above, it's a bit odd that we allow specifying a deprecation version for all platforms since the different platforms don't need to align their versions and in practice don't do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed now
Tests/SwiftDocCTests/Test Bundles/AvailabilityBundle.docc/ComplexAvailableDeprecated.md
Outdated
Show resolved
Hide resolved
Sorry for the wait. I left some initial feedback and in the upcoming days I'll do another round of reviews. |
@mustiikhalil please update the |
I'll look at it during the weekend. |
dda5175
to
57141c6
Compare
c9d5867
to
4e8cb9d
Compare
4e8cb9d
to
f096f6a
Compare
f096f6a
to
0b394a2
Compare
@d-ronnqvist what can I do to make this PR move forward? |
Sorry for the very, very long delay. In the Documentation Workgroup meeting on June 3, one of the topics that we discussed was how "*" availability should be displayed on the page to better reflect that a large number of packages work on Linux and Windows and anywhere else where Swift is supported. We didn't come to any specific conclusions about how it should be displayed but decided that it should be pitched on the Swift Forums where the broader community can be part of the conversation. The goal is to discuss and hopefully implement those changes in the 6.0 timeframe but it may still take a few weeks before everyone agrees on how it should be displayed and how it should behave. If you don't want to wait for that discussion to happen, it may be good to separate the |
No worries
If that's the case let's keep this PR tiny, as in adding support for deprecated. And create an issue for the "any" which I would be happy to work on when the Specs for it are ready. I will simply revert the changes when parsing the "*" operator in the platforms enum. And revert a couple of the tests, comments that removed the mention of What do you think? |
That sounds like a good plan |
Adding deprecated argument to the directive @available and updates documentation to include the new argument
0b394a2
to
2fcf1fd
Compare
|
||
// also test for giving no platform | ||
for args in validArguments { | ||
try assertValidAvailability(source: "@Available(\(args))") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this code since it was never called because validArguments
was always empty before. However now we have deprecated
as a value within the array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. It looks like this loop wasn't really testing what the comment says.
It also seems unnecessary to have 3 loops in this test. The entire "My Package" loop could be removed if there was an checkPlatforms.append("\"My Package\"")
before the first loop.
@d-ronnqvist Super sorry for the late update, but this should be good now. Only added a simple parameter and reverted all the instances where I removed the wild card "*" comments. |
|
||
// also test for giving no platform | ||
for args in validArguments { | ||
try assertValidAvailability(source: "@Available(\(args))") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. It looks like this loop wasn't really testing what the comment says.
It also seems unnecessary to have 3 loops in this test. The entire "My Package" loop could be removed if there was an checkPlatforms.append("\"My Package\"")
before the first loop.
@swift-ci please test |
This PR no longer adds support for "any" / "*" availability
@swift-ci please test |
Adding deprecated argument to the directive @available and updates documentation to include the new argument
Adding deprecated argument to the directive `@available`. rdar://130516145
Summary
Adding
deprecated
argument to the directive@Available
, this is done by adding a fielddeprecated
to theMetadata.Availability
and also addressing all the comments that are related to the issue within the codebase.This PR also include the
.any
platform for@Available
, which can be used as follows:Closes #441
Testing
Adding the following metadata into
SwiftDocC/Directives.md
Using swift-docc-render-artifact
Steps:
Add the above metadata to
Sources/SwiftDocC/SwiftDocC.docc/SwiftDocC/Directives.md
../bin/preview-docs SwiftDocC
Ensure that in the documentation list the "Directives" shows a deprecated tag
Checklist
./bin/test
script and it succeeded(Not sure how to update the docs properly)