Make it easier to support other link disambiguation in the future #824
Conversation
|
|
||
| enum Disambiguation { | ||
| /// This path component uses a combination of kind and hash disambiguation | ||
| case kindAndHash(kind: Substring?, hash: Substring?) |
There was a problem hiding this comment.
Would it make sense to add kind and hash accessor functions or calculated properties in this enum that return Substring? That would allow you to further simplify the code that uses these values.
There was a problem hiding this comment.
Maybe. I wouldn't want it to work against the goal of making it easier to add other types of link disambiguation.
There was a problem hiding this comment.
My goal is to break apart #643 into 3 pieces: this PR, a follow up PR that changes how disambiguated elements are stored in the path hierarchy, and finally the new link disambiguation syntax. The idea is that we can land the two refactor PRs without changing any behavior and that makes the 3rd (new feature) PR smaller and less likely to have complicated merge conflicts.
| var kind: String? | ||
| var hash: String? | ||
| switch disambiguation { | ||
| case .kindAndHash(kind: let maybeKind, hash: let maybeHash): | ||
| kind = maybeKind.map(String.init) | ||
| hash = maybeHash.map(String.init) | ||
| case nil: | ||
| kind = nil | ||
| hash = nil | ||
| } |
There was a problem hiding this comment.
You could move this snippet from here into PathHierarchy.PathComponent.Disambiguation and separate it into two calculated properties. Then I think you could simplify this function even more.
There was a problem hiding this comment.
I've already removed this code in the follow up change—for which I haven't opened a PR yet—that updates how the disambiguated elements are stored in the path hierarchy.
| kind = nil | ||
| hash = nil | ||
| } | ||
|
|
||
| if let kind = kind { |
There was a problem hiding this comment.
For example this line here could just read if let kind = disambiguation.kind
| guard name == component.name else { | ||
| return false | ||
| else if let symbol = symbol, let disambiguation = component.disambiguation { | ||
| switch disambiguation { |
There was a problem hiding this comment.
And here for example you might be able to avoid the switch statement and avoid referring to the enum case directly.
|
@swift-ci please test |
Bug/issue #, if applicable:
Summary
This handpicks some of the changes from #643 to make it easier to support other types of link disambiguation without adding any new link disambiguation.
Specifically, this wraps the path component disambiguation as its own type instead of storing the values directly on the path component. This means that in the future, if we added a new type of link disambiguation—like in #643—then all the places that process path component disambiguation would already be switching over the type of disambiguation.
A follow up PR will handpick some more changes from #643 to change how the path hierarchy internally stores its disambiguated elements.
Dependencies
None
Testing
Nothing in particular.
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
AddedUpdated tests./bin/testscript and it succeeded[ ] Updated documentation if necessary