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

Document using DOCC_EXEC in SwiftPM #653

Merged

Conversation

natikgadzhi
Copy link
Contributor

Bug/issue #, if applicable: N/A

Summary

This pull request adds a paragraph in README describing how to invoke a locally built docc when using SwiftPM, setting DOCC_EXEC env variable.

I was working on #652 and debugged my ideas that way, thought I'd add a doc entry for that.

Dependencies

None.

Testing

To test whether it works:

  1. Build docc as usual
  2. run DOCC_EXEC=./build/debug/docc swift generate-documentation --target=TARGET

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

README.md Outdated Show resolved Hide resolved
README.md Outdated
CLI. SwiftPM will try to read `DOCC_EXEC` environment variable value, and use
the path you provded if it's set.

1. In your test `Package.swift`, add a dependency on `swift-docc-plugin`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Feel a little weird for me. IMO, swift-docc developer should make direct call to docc instead of relying on swift-docc-plugin.

eg. docc convert/preview xx --additional-symbol-graph-dir xx

@natikgadzhi
Copy link
Contributor Author

natikgadzhi commented Jul 5, 2023 via email

Copy link
Contributor Author

@natikgadzhi natikgadzhi left a comment

Choose a reason for hiding this comment

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

I don't like people who walk into a repo and do a "Hey you had some trailing space!", but I am that person now, it seems.

  1. Cleaned up the langage, thank you to @Kyle-Ye for noticing a typo. It's in this commit so it's easier to review: 1d61565
  2. A separate commit with formatting cleanup: 6e31c34 — I noticed my editor apply that, so I did a partial commit with an actually useful documentation piece separately, and then committed the formatting to, while I'm there.

README.md Outdated Show resolved Hide resolved
@natikgadzhi natikgadzhi force-pushed the internal/readme-docc-exec-env branch from 6e31c34 to ad29e66 Compare July 6, 2023 16:29
README.md Outdated
@@ -1,9 +1,9 @@
# Swift-DocC

Swift-DocC is a documentation compiler for Swift frameworks and packages aimed
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 like people who walk into a repo and do a "Hey you had some trailing space!", but I am that person now, it seems.

The "trailing space" is here on purpose. For some linter rule or Apple's writing guidance, we break the whole sentence into "2" lines(Line 3 and Line 4). But on the markdown side, it was still rendered as one line since there is no Hardbreak.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing it out — I didn't know about that. I've removed the whole formatting commit — it feels like I don't know enough about internal systems to make it warranted and safe, and I don't feel there's enough value for beginner engineers reading the docs, or for the core team, to justify spending your time on reviewing it further.

@natikgadzhi natikgadzhi force-pushed the internal/readme-docc-exec-env branch from ad29e66 to 0d08cc0 Compare July 7, 2023 01:02
@Kyle-Ye Kyle-Ye mentioned this pull request Jul 7, 2023
3 tasks
Copy link
Contributor

@ethan-kusters ethan-kusters left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you for making this change – I rely on this workflow of testing DocC via the SwiftPM plugin a lot and I think other contributors will appreciate having this documented.

I left some suggestions on wording/formatting but nothing blocking. Let me know what you think.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
Comment on lines 150 to 151
You can also test a locally built version of Swift-DocC using Swift Package Manager
CLI. SwiftPM will try to read `DOCC_EXEC` environment variable value, and use
Copy link
Contributor

@ethan-kusters ethan-kusters Jul 7, 2023

Choose a reason for hiding this comment

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

What do you think of avoiding the CLI abbreviation here? I think it's possible some readers won't be familiar with it.

Suggested change
You can also test a locally built version of Swift-DocC using Swift Package Manager
CLI. SwiftPM will try to read `DOCC_EXEC` environment variable value, and use
You can also test a locally built version of Swift-DocC using the Swift Package Manager from the command line.
SwiftPM will try to read `DOCC_EXEC` environment variable value, and use

README.md Outdated Show resolved Hide resolved
@natikgadzhi
Copy link
Contributor Author

@ethan-kusters, thank you for the review, and for the recommendations! Cleaned things up, should be ready to merge now.

@natikgadzhi natikgadzhi force-pushed the internal/readme-docc-exec-env branch from 22605bf to d2b54c2 Compare July 7, 2023 20:54
@natikgadzhi
Copy link
Contributor Author

Huh, TIL if I rebase the branch in GitHub UI, it removes the signature from my commits ;(

Copy link
Contributor

@ethan-kusters ethan-kusters left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you!

@ethan-kusters
Copy link
Contributor

@swift-ci please test

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.

3 participants