Add TutorialDirectiveMisuseChecker #278

Kyle-Ye · 2022-06-05T04:33:29Z

Bug/issue #, if applicable:

Close #274

Summary

Add a warning for directive usage on non-tutorial files (Checker will only run on Articles).

Add possible altertive solutions for Image and Video Directive.

Test

Installing into Xcode

You can test a locally built version of Swift-DocC in Xcode 13 or later by setting the DOCC_EXEC build setting to the path of your local docc:

Select the project in the Project Navigator.

In the Build Settings tab, click '+' and then 'Add User-Defined Setting'.

Create a build setting DOCC_EXEC with the value set to /path/to/docc.

The next time you invoke a documentation build with the "Build Documentation" button in Xcode's Product menu, your custom docc will be used for the build. You can confirm that your custom docc is being used by opening the latest build log in Xcode's report navigator and expanding the "Compile documentation" step.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

Added tests
Ran the ./bin/test script and it succeeded
Updated documentation if necessary

Kyle-Ye · 2022-06-05T08:52:50Z

swiftlang/swift-markdown#50 solves the swiftlang/swift-markdown#49 issue, remove the workaround implementation

Kyle-Ye · 2022-06-05T08:53:40Z

@swift-ci please test

ktoso

Looks nice to me, thanks!

Kyle-Ye · 2022-06-05T09:03:45Z

swiftlang/swift-markdown#50
@swift-ci please smoke test

Kyle-Ye · 2022-06-05T09:27:29Z

swiftlang/swift-markdown#50
@swift-ci please smoke test

franklinsch · 2022-06-06T11:43:33Z

Sources/SwiftDocC/Checker/Checkers/TutorialDirectiveMisuseChecker.swift

+/*
+ This source file is part of the Swift.org open source project
+
+ Copyright (c) 2021 Apple Inc. and the Swift project authors


Suggested change

Copyright (c) 2021 Apple Inc. and the Swift project authors

Copyright (c) 2022 Apple Inc. and the Swift project authors

franklinsch · 2022-06-06T11:50:32Z

Sources/SwiftDocC/Checker/Checkers/TutorialDirectiveMisuseChecker.swift

+        let solutions: [Solution]
+
+        switch blockDirective.name {
+        case ImageMedia.directiveName:


Should we also diagnose Video and the other Tutorial-specific directives? @binamaniar added this property recently which should be helpful: https://github.com/apple/swift-docc/pull/154/files#diff-cd174ae1fe7d1732ca971afb59e3ad1575275faa1701aea03c78e8711b505ccaR22

We'd need to subtract Comment, DisplayName, DocumentationExtension, Metadata, Snippet, Redirect, DeprecationSummary at least since those can be used in Markdown articles.

Video is already diagnosed. But only @image will have solutions. Since Video does not have an equivalent in Markdown form.

Oh, I did not know they are supported in Markdown articles. I'll substract them to not produce a diagnost.

one way to approach this is have a list of all the directives which are not supported in Markdown and provide a diagnostics for that. i see right now, Image case is covered and rest goes over to default.

One potential enhancement that would be nice here is to have separate lists for documentation extension files (markdown files with a symbol link in the level 1 heading) and articles (other markdown files). DisplayName, DocumentationExtension, and DeprecationSummary are only allowed in documentation extension files.

franklinsch · 2022-06-06T11:53:08Z

Sources/SwiftDocC/Checker/Checkers/TutorialDirectiveMisuseChecker.swift

+                severity: .warning,
+                range: blockRange,
+                identifier: "org.swift.docc.TutorialDirectiveMisuse",
+                summary: #""@\#(blockDirective.name)" directive is not support on normal markdown file."#,


It would be good to use a similar style to this diagnostic we already have: https://github.com/apple/swift-docc/blob/main/Sources/SwiftDocC/Model/DocumentationNode.swift#L396, e.g., "Directive 'Image' is not supported in…".

Rather than "normal markdown file", the DocC naming is either "article" or "documentation extension". Can we know easily what kind of markdown file this Checker being run on so that we show the right text?

The checkers will only run on Articles, but it seems @image is only supported on Tutorials. Maybe we need run this checker on "documentation extension" too.

Gotcha. Yes I think it would be good to run this on documentation extensions as well.

Sources/SwiftDocC/Checker/Checkers/TutorialDirectiveMisuseChecker.swift

franklinsch · 2022-06-06T11:55:40Z

Tests/SwiftDocCTests/Semantics/ChoiceTests.swift

@@ -233,7 +233,7 @@ Choice @1:1-9:2 isCorrect: true
                let expectedDump = """
 Choice @1:1-7:2 isCorrect: true
 ├─ MarkupContainer (empty)
-├─ ImageMedia @2:4-2:11 source: 'ResourceReference(bundleIdentifier: "org.swift.docc.example", path: "blah.png")' altText: 'blah'
+├─ ImageMedia @2:4-2:39 source: 'ResourceReference(bundleIdentifier: "org.swift.docc.example", path: "blah.png")' altText: 'blah'


Hmm, why did these column numbers change?

See more info here swiftlang/swift-markdown#49.

After swiftlang/swift-markdown#50, this column needs to be changed. Maybe we can discuss this change on swiftlang/swift-markdown#50 for whether this change should be made.

Ah I see. Yes I think we should open a separate PR for this column changes, if we take the swift-markdown change.

So on this PR, we use the original buggy workaround and then a PR both on swift-markdown and swift-docc to fix this?

Hm maybe I'm misunderstanding, but do these tests currently fail on main without the Swift-Markdown change? If they pass, then would these changes not make the tests fail?

This will not fail on main of Swift-Markdown. But after swiftlang/swift-markdown#50, this will fail. And this PR need swiftlang/swift-markdown#50.

Gotcha. It seems like the dependency swiftlang/swift-markdown#50 is unrelated to the work in this PR, so I think we should decouple them. We can keep this PR for the Tutorial directive–related changes, and create another Swift-DocC one to adopt the changes introduced by the Swift-Markdown PR. That way landing this PR isn't blocked by the Markdown change. What do you think?

This PR needs the feature of swiftlang/swift-markdown#50, we certainly can decouple them. But if we do so, we'll have to use a buggy implementation as a temporary workaround. See the full context on swiftlang/swift-markdown#49 of other.

I think it would be beneficial to decouple them so that we can merge the swift-markdown PR independently from this one. Otherwise we won't be able to merge the Swift-Markdown changes without this PR

ethan-kusters · 2022-06-07T19:52:46Z

I added a comment on the issue but want to link it here for visibility as well:

@Image seems to be ignored in Articles, one has to use ![]() #274 (comment)

I just want to confirm that we think this is actually the right fix. Personally, I think it's reasonable for folks to use @Image in regular .md files.

Kyle-Ye · 2022-10-05T02:28:19Z

Close since #381 is merged

Kyle-Ye mentioned this pull request Jun 5, 2022

BlockDirective range issue swiftlang/swift-markdown#49

Closed

Kyle-Ye force-pushed the directive-check branch 2 times, most recently from 8a050ca to e4a9310 Compare June 5, 2022 06:54

Kyle-Ye marked this pull request as ready for review June 5, 2022 06:54

Kyle-Ye added the new feature New features or new functionality label Jun 5, 2022

Kyle-Ye requested review from ethan-kusters and franklinsch June 5, 2022 06:55

ktoso reviewed Jun 5, 2022

View reviewed changes

franklinsch requested a review from binamaniar June 6, 2022 11:43

franklinsch reviewed Jun 6, 2022

View reviewed changes

Kyle-Ye added this to the Swift 5.7 milestone Jun 6, 2022

This was referenced Jun 11, 2022

Update test data to fit the swift-markdown #295

Merged

@Image seems to be ignored in Articles, one has to use ![]() #274

Closed

Add TutorialDirectiveMisuseChecker

60911e3

Kyle-Ye force-pushed the directive-check branch from ef97462 to 60911e3 Compare June 15, 2022 12:35

Kyle-Ye marked this pull request as draft June 15, 2022 12:37

Kyle-Ye closed this Oct 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TutorialDirectiveMisuseChecker #278

Add TutorialDirectiveMisuseChecker #278

Kyle-Ye commented Jun 5, 2022 •

edited

Loading

Kyle-Ye commented Jun 5, 2022

Kyle-Ye commented Jun 5, 2022

ktoso left a comment

Kyle-Ye commented Jun 5, 2022

Kyle-Ye commented Jun 5, 2022

franklinsch Jun 6, 2022

Kyle-Ye Jun 6, 2022

franklinsch Jun 6, 2022

Kyle-Ye Jun 6, 2022

binamaniar Jun 7, 2022 •

edited

Loading

d-ronnqvist Aug 1, 2022

franklinsch Jun 6, 2022

Kyle-Ye Jun 6, 2022

franklinsch Jun 6, 2022

franklinsch Jun 6, 2022

Kyle-Ye Jun 6, 2022

franklinsch Jun 6, 2022

Kyle-Ye Jun 6, 2022

franklinsch Jun 6, 2022

Kyle-Ye Jun 6, 2022

franklinsch Jun 6, 2022

Kyle-Ye Jun 6, 2022 •

edited

Loading

franklinsch Jun 7, 2022

ethan-kusters commented Jun 7, 2022

Kyle-Ye commented Oct 5, 2022

	Copyright (c) 2021 Apple Inc. and the Swift project authors
	Copyright (c) 2022 Apple Inc. and the Swift project authors

Add TutorialDirectiveMisuseChecker #278

Add TutorialDirectiveMisuseChecker #278

Conversation

Kyle-Ye commented Jun 5, 2022 • edited Loading

Summary

Test

Installing into Xcode

Checklist

Kyle-Ye commented Jun 5, 2022

Kyle-Ye commented Jun 5, 2022

ktoso left a comment

Choose a reason for hiding this comment

Kyle-Ye commented Jun 5, 2022

Kyle-Ye commented Jun 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

binamaniar Jun 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kyle-Ye Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethan-kusters commented Jun 7, 2022

Kyle-Ye commented Oct 5, 2022

Kyle-Ye commented Jun 5, 2022 •

edited

Loading

binamaniar Jun 7, 2022 •

edited

Loading

Kyle-Ye Jun 6, 2022 •

edited

Loading