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

Adding deprecated to @Available directive #851

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

Conversation

mustiikhalil
Copy link

@mustiikhalil mustiikhalil commented Mar 5, 2024

Summary

Adding deprecated argument to the directive @Available, this is done by adding a field deprecated to the Metadata.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:

@Available(*, introduced: "1.0")
@Available(*, introduced: "1.0", deprecated: "10.0")

Closes #441

Testing

Adding the following metadata into SwiftDocC/Directives.md

@Metadata {
    @Available("Swift DocC", introduced: "1.0", deprecated: "2.0")
}

Using swift-docc-render-artifact

Steps:

  1. Add the above metadata to Sources/SwiftDocC/SwiftDocC.docc/SwiftDocC/Directives.md.

  2. ./bin/preview-docs SwiftDocC

  3. Ensure that in the documentation list the "Directives" shows a deprecated tag

Screenshot 2024-03-08 at 4 09 05 PM
  1. Ensure that the "Directives" page is printed with an availability callout for "Swift DocC" which contains a introduced and deprecated versions
Screenshot 2024-03-08 at 4 09 12 PM

Checklist

  • Added tests
  • Ran the ./bin/test script and it succeeded
  • Updated documentation if necessary
    (Not sure how to update the docs properly)

@mustiikhalil mustiikhalil force-pushed the issue-441-add-deprecated-argument branch from a80b723 to c2d167f Compare March 5, 2024 18:44
@mustiikhalil mustiikhalil force-pushed the issue-441-add-deprecated-argument branch 2 times, most recently from d688517 to 8fff1c0 Compare March 7, 2024 17:28
@mustiikhalil mustiikhalil marked this pull request as ready for review March 7, 2024 17:28
@sofiaromorales
Copy link
Member

sofiaromorales commented Mar 8, 2024

@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?

@mustiikhalil
Copy link
Author

mustiikhalil commented Mar 8, 2024

Could you please add the Steps to reproduce section to the PR template, including the expected behaviour

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

@mustiikhalil mustiikhalil force-pushed the issue-441-add-deprecated-argument branch 2 times, most recently from 552c4bf to 86da846 Compare March 12, 2024 17:32
@mustiikhalil mustiikhalil force-pushed the issue-441-add-deprecated-argument branch 2 times, most recently from 02e741d to b5cc26e Compare March 18, 2024 23:27
@mustiikhalil
Copy link
Author

@sofiaromorales sorry for the ping, but would love to know what's next to get this PR moving forward

Sources/SwiftDocC/Semantics/Metadata/Availability.swift Outdated Show resolved Hide resolved
@@ -77,6 +74,7 @@ extension Metadata {
case .iOS: return "iOS"
case .watchOS: return "watchOS"
case .tvOS: return "tvOS"
case .any: return "*"
Copy link
Member

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?

Copy link
Author

Choose a reason for hiding this comment

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

It would look something like the screenshot below:

Screenshot 2024-03-22 at 5 33 26 PM

Copy link
Contributor

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.

Copy link
Author

Choose a reason for hiding this comment

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

If that's the case what would be the correct representation to say that it is deprecated on all platforms?

Where as a specific case of deprecation might look like this for example:

Screenshot 2024-03-28 at 10 10 36 PM

Copy link
Contributor

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.

Copy link
Author

Choose a reason for hiding this comment

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

Ah that makes sense, I'll update the PR as soon as possible with removing the *. And thus aligning with screenshot sent below/ in the previous comment:

Screenshot 2024-03-28 at 10 10 36 PM

Copy link
Author

Choose a reason for hiding this comment

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

Should be fixed now

@sofiaromorales
Copy link
Member

@sofiaromorales sorry for the ping, but would love to know what's next to get this PR moving forward

Sorry for the wait. I left some initial feedback and in the upcoming days I'll do another round of reviews.

@sofiaromorales
Copy link
Member

Updated documentation if necessary
(Not sure how to update the docs properly)

@mustiikhalil please update the Availability DocC comment to properly document this change.

@mustiikhalil
Copy link
Author

I'll look at it during the weekend.

@mustiikhalil mustiikhalil force-pushed the issue-441-add-deprecated-argument branch 2 times, most recently from dda5175 to 57141c6 Compare March 22, 2024 16:47
@mustiikhalil mustiikhalil force-pushed the issue-441-add-deprecated-argument branch from c9d5867 to 4e8cb9d Compare April 4, 2024 15:16
Adding deprecated argument to the directive @available and updates documentation to include the new argument
Removes unneeded md file, and updates documentation
@mustiikhalil mustiikhalil force-pushed the issue-441-add-deprecated-argument branch from 4e8cb9d to f096f6a Compare May 15, 2024 05:35
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.

Add deprecated version argument to the @Available directive
3 participants