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

minimal DocC markdown to order Logger.Levels #225

Merged
merged 9 commits into from
Jul 20, 2022
Merged

Conversation

heckj
Copy link
Contributor

@heckj heckj commented Jul 19, 2022

minimal DocC markdown to order Logger.Levels

Motivation:

When viewed in documentation generated by DocC, the enumeration levels
are ordered alphabetically, removintg the implied ordering in the
source. The written overview of the enum relies on that ordering to make
sense.

Modifications:

Adding a single file that provides structure to Logging/Logger/Levels
within DocC generated documentation.

Result:

When imported into a project that leverages Swift 5.6 (such as Vapor),
the generated documentation for the levels provided by
Logging/Logger/Levels are displayed in the order intended/expected by
the documentation. Resolves #224.

Motivation:

When viewed in documentation generated by DocC, the enumeration levels
are ordered alphabetically, removintg the implied ordering in the
source. The written overview of the enum relies on that ordering to make
sense.

Modifications:

Adding a single file that provides structure to Logging/Logger/Levels
within DocC generated documentation.

Result:

When imported into a project that leverages Swift 5.6 (such as Vapor),
the generated documentation for the levels provided by
Logging/Logger/Levels are displayed in the order intended/expected by
the documentation.
@swift-server-bot
Copy link

Can one of the admins verify this patch?

11 similar comments
@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@ktoso
Copy link
Member

ktoso commented Jul 19, 2022

@swift-server-bot test this please

Motivation:

Per DocC team, an updated version is required for the DocC changes
to be picked up and reflected in generated dcumentation, even for
upstream projects already using swift 5.6 or later.

Modifications:

Adding Package.Swift@5.6 with the swift version updated.

Result:

Hopefully, documentation updates in DocC generated content will be
reflected.
@ktoso
Copy link
Member

ktoso commented Jul 20, 2022

Cool, thanks! We worked through this and sadly needed to add a new Package-swift5.6.swift so docc can actually find the docc files, but it works well now:

Screenshot 2022-07-20 at 9 06 24

@ktoso
Copy link
Member

ktoso commented Jul 20, 2022

@swift-server-bot test this please

@ktoso
Copy link
Member

ktoso commented Jul 20, 2022

Heh seems Ci now has issues due to

error: emit-module command failed with exit code 1 (use -v to see invocation)[25/28] Emitting module swift_logPackageTests
/code/Tests/LinuxMain.swift:28:1: error: expressions are not allowed at the top level
XCTMain([
^
[26/28] Compiling swift_logPackageTests LinuxMain.swift
/code/Tests/LinuxMain.swift:28:1: error: expressions are not allowed at the top level
XCTMain([
^
error: fatalError

on 5.6 builds -- so we may need to #if them out

Motivation:

To appease CI, added #if statements into generated LinuxMain
content to handle conditional compilation with Swift 5.6 and later.

Modifications:

Added #if statements to test generating script and re-ran the script

Result:

CI will hopefully pass.
@ktoso
Copy link
Member

ktoso commented Jul 20, 2022

@swift-server-bot add to allowlist

@ktoso
Copy link
Member

ktoso commented Jul 20, 2022

@swift-server-bot test this please

@ktoso
Copy link
Member

ktoso commented Jul 20, 2022

@swift-server-bot test this please

@ktoso
Copy link
Member

ktoso commented Jul 20, 2022

I'm looking into how to have all swift versions survive this -- will push this over the finish line, thanks for the PR @heckj !

@ktoso
Copy link
Member

ktoso commented Jul 20, 2022

@swift-server-bot test this please

@@ -71,6 +71,7 @@ def createExtensionFile(fileName, classes)
file.write "\n"

for classArray in classes
file.write "#if swift(<5.6)\n"
Copy link
Member

Choose a reason for hiding this comment

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

these set of changes seem redundant to me. can you provide more background on what issue these are solving?

Copy link
Member

@ktoso ktoso Jul 20, 2022

Choose a reason for hiding this comment

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

It's a swiftpm regression, I'm talking with Boris about it

Swift seems to be unable to compile these files anymore - seems to have regressed recently somewhere.
The error is:

error: emit-module command failed with exit code 1 (use -v to see invocation)[25/28] Emitting module swift_logPackageTests
/code/Tests/LinuxMain.swift:28:1: error: expressions are not allowed at the top level
XCTMain([
^
[26/28] Compiling swift_logPackageTests LinuxMain.swift
/code/Tests/LinuxMain.swift:28:1: error: expressions are not allowed at the top level
XCTMain([
^
error: fatalError

rdar://97297732

@tomerd
Copy link
Member

tomerd commented Jul 20, 2022

thanks for the contribution @heckj

I am a bit surprised to see the formatting and test generation changes. what issue did you run into that triggered these changes?

Copy link
Member

@tomerd tomerd left a comment

Choose a reason for hiding this comment

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

lets try to find a simpler way to deal with the test generation issues and rollback the formatting changes.

@ktoso
Copy link
Member

ktoso commented Jul 20, 2022

Okey I think I managed to minimize the change and fix the LinuxMain issue with only flags passed to SwiftPM on 5.6+

Have a look folks, I think this'll be good; And we can fix the SwiftPM issue independently as well even on a linux only 5.6 release perhaps.

@ktoso
Copy link
Member

ktoso commented Jul 20, 2022

Waiting this out a little bit while we investigate the LinuxMain issue to see if we can solve it more cleanly -- we'll take this over and merge once we have a plan here -- thanks for the PR @heckj !

@ktoso ktoso self-assigned this Jul 20, 2022
Copy link
Member

@tomerd tomerd left a comment

Choose a reason for hiding this comment

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

this looks much nicer now!

@ktoso
Copy link
Member

ktoso commented Jul 20, 2022

Okey we're clear here and we'll look into LinuxMain issues separately -- thank you @heckj !

@ktoso ktoso merged commit 8fc79ca into apple:main Jul 20, 2022
@heckj heckj deleted the docc-ordering branch July 20, 2022 05:50
@heckj
Copy link
Contributor Author

heckj commented Jul 20, 2022

Thanks! Appreciate all the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants