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

Fix inverse logic in hierarchy based link resolver environment opt-out #548

Merged
merged 2 commits into from
Apr 12, 2023

Conversation

d-ronnqvist
Copy link
Contributor

Bug/issue #, if applicable:

Summary

This fixes an issue where the opt-logic for the new link resolver was inverted.

Dependencies

None.

Testing

  • Run swift run docc convert /path/to/SomeBundle.docc with DOCC_USE_HIERARCHY_BASED_LINK_RESOLVER=YES.
    DocC should use the hierarchy based resolver.
  • Run swift run docc convert /path/to/SomeBundle.docc without DOCC_USE_HIERARCHY_BASED_LINK_RESOLVER set.
    DocC should use the hierarchy based resolver.
  • Run swift run docc convert /path/to/SomeBundle.docc with DOCC_USE_HIERARCHY_BASED_LINK_RESOLVER=NO.
    DocC shouldn't use the hierarchy based resolver.

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

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@QuietMisdreavus
Copy link
Contributor

What do you think about having the environment variable override the user default here? I ran into an issue that only showed up on CI because i had the user default set, so bin/test didn't actually use the old link resolver and covered up an issue.

@d-ronnqvist
Copy link
Contributor Author

What do you think about having the environment variable override the user default here? I ran into an issue that only showed up on CI because i had the user default set, so bin/test didn't actually use the old link resolver and covered up an issue.

I think that's a good idea. There's still the chance that someone would set both to different values and get confused by the result but I think it's less likely that someone would forget an environmental variable than a user default.

We could do something more sophisticated—like warning if both configuration values conflict—but the plan is to remove this configuration after Swift 5.9 so I don't think that doing more than changing the order would be worth it here.

Also, support more values as true/false for environment configuration.
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist merged commit 27e0cd6 into swiftlang:main Apr 12, 2023
@d-ronnqvist d-ronnqvist deleted the link-resolver-opt-out-env branch April 12, 2023 23:58
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.

2 participants