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

[SR-15634] Remove need to pass --fallback-... CLI arguments in most cases #194

Closed
d-ronnqvist opened this issue Dec 22, 2021 · 9 comments · Fixed by #391
Closed

[SR-15634] Remove need to pass --fallback-... CLI arguments in most cases #194

d-ronnqvist opened this issue Dec 22, 2021 · 9 comments · Fixed by #391
Assignees
Labels
enhancement Improvements or enhancements to existing functionality good first issue Good for newcomers

Comments

@d-ronnqvist
Copy link
Contributor

Previous ID SR-15634
Radar rdar://problem/86819483
Original Reporter @d-ronnqvist
Type Improvement
Additional Detail from JIRA
Votes 0
Component/s Swift-DocC
Labels Improvement, StarterBug
Assignee waheedNowLovesSwift (JIRA)
Priority Medium

md5: 675dc1ff1939386d0c186c42c940543c

Issue Description:

Swift-DocC requires some minimal information about the documentation that it's building to name and identify the documentation. This information can be specified in a couple of different ways:

  • If the documentation has a DocC catalog (a folder with a .docc extension) with an Info.plist file in it, then the documentation's name and identifier can be read from the content of that identifier.

  • If the documentation doesn't have a DocC catalog, or the catalog doesn't have an Info.plist file, then the documentation's name and identifier can be provided as two command line arguments (----``fallback-display-name and --``fallback-bundle-identifier). These arguments are prefixed with "fallback" because they are only used when a value can't be retrieved from the DocC catalog's Info.plist file (for example if that file doesn't exist).

The documentation's identifier doesn't need to follow a specific format. That said, the identifier needs to be unique to the current documentation build. In almost all cases documentation is built separately for each "bundle", so uniqueness isn't an issue. Further, even when multiple "bundles" are built together their display name are most often unique enough.

A documentation "bundle" is the term for the in-memory representation about a framework or other project. One DocC catalog corresponds to one documentation bundle, but a bundle can also be created from the module information in symbol graph files when there's no DocC catalog.

Because of this, a suitable documentation identifier can almost always be derived from the documentation's display name. When that derived identifier isn't unique, this can be identified and diagnosed and that issue can be resolved by providing a unique identifier either as an Info.plist value or as a custom --``fallback-bundle-identifier argument.

Further, a suitable documentation display name can almost always be derived from other information in the documentation "bundle". A documentation bundle consist of either a DocC catalog or a collection of symbol graph files, or both.

Most documentation is about a single module. In this case, when all the bundle's symbol graph files are about the same module, then that module name is a good display name in almost all cases. When it's not, a custom name can be provided as a --fallback-display-name argument.

If the bundle contains symbol graph files about more than one module or if the bundle doesn't contain any symbol graph files, then it's less clear what a good display name would be. However, since the developer need to provide a custom display name if none is derived but only needs to provide a custom display name if the derived one is incorrect, I feel that deriving a display name that's good enough in many cases still provide value here. In this situation, I feel that deriving the documentation from the DocC catalogs filename (without the file extension) is likely to be a good enough value in many cases.

Since the documentation's version information is already optional, making the above changes to derive the documentation's identifier and display name would remove most of the cases when developer's need to pass --fallback... CLI arguments.

Summary

If a documentation bundle has no specified identifier, derive the identifier from the bundle's display name.

If a documentation bundle has no specified display name:

  • and it has symbol graph files about a single module, derive the display name from the module's name

  • and it has a DocC catalog, derive the display name from the catalog's filename (without the file extension)

  • otherwise (if the bundle has no DocC catalog and symbol graphs about multiple modules, raise the same error as today that a display is missing name and needs to be provided.

@d-ronnqvist
Copy link
Contributor Author

@swift-ci create

@d-ronnqvist
Copy link
Contributor Author

To implement what's described in the Description there are few different places to look at:

The display name and identifier decoded in init(with:bundleDiscoveryOptions: in DocumentationBundle+Info.swift
That's a good place to default to the display name when the identifier is missing. Depending on this the decoding is done, requiredKeys may also need to change.

That code doesn't know what the DocC catalog is named or what the modules are named—they know the symbols graph URLs but would need to read and decode the files to find the module name—so the last resort derived display name would need to be passed there or otherwise be made accessible somehow.

There are different code paths for creating documentation "bundles" when there's a DocC catalog and when there isn't.

When there's a DocC catalog the code path that creates the bundle is in createBundle(::options🙂 in DataProviderBundleDiscovery.swift

When there's no DocC catalog the bundle is created in GeneratedDataProvider. It also has some code to read the module names out of the symbol graph files.

DocumentationBundleInfoTests tests the existing decoding (which would be updated to cover the logic for deriving values) but it could also be good to test the full flow in ConvertActionTests by passing a few different variations of bundle discovery options both with and without a DocC catalog (the documentationBundleURL: argument to ConvertAction

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 3, 2022
@d-ronnqvist d-ronnqvist added good first issue Good for newcomers and removed StarterBug labels May 3, 2022
@arthurcro
Copy link
Contributor

Hey! 👋 As part of the Swift Mentorship Program and with the help of my mentor, I'm hoping to make my first contribution to an open-source project. I would love to give this issue a try! 🙂

@d-ronnqvist
Copy link
Contributor Author

That's great to hear. Feel free to reach out if you have any questions.

@Kyle-Ye
Copy link
Collaborator

Kyle-Ye commented Sep 25, 2022

Hi @arthurcro, are you still working on this? Let us know we can help!

(This is indeed a annoying issue for the user site, and hope we can solve it ASAP)

@arthurcro
Copy link
Contributor

Hey @Kyle-Ye! My apologies for the delay in closing this issue. I actually have something working on a fork but I got stuck adding a test covering the full flow in ConvertActionTests and I haven't had the opportunity to pick it up again. I'm not quite sure how to get started adding a test there. I'd love some help with this if you have time! 🙂

@Kyle-Ye
Copy link
Collaborator

Kyle-Ye commented Sep 25, 2022

Hey @Kyle-Ye! My apologies for the delay in closing this issue. I actually have something working on a fork but I got stuck adding a test covering the full flow in ConvertActionTests and I haven't had the opportunity to pick it up again. I'm not quite sure how to get started adding a test there. I'd love some help with this if you have time! 🙂

Would you mind pointing out which branch you were working? If I guess correctly, should it be https://github.com/arthurcro/swift-docc/tree/arthurcro/improve-deriving-cli-arguments

@Kyle-Ye
Copy link
Collaborator

Kyle-Ye commented Sep 25, 2022

I'd love some help with this if you have time!

Maybe you can start to create a draft PR. So that we can comment on it and give you some feedback

@arthurcro
Copy link
Contributor

Thanks for the advice! I just opened a draft PR #391, I look forward to your feedback!

@Kyle-Ye Kyle-Ye linked a pull request Sep 30, 2022 that will close this issue
3 tasks
@Kyle-Ye Kyle-Ye closed this as completed Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements or enhancements to existing functionality good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants