Skip to content

Conversation

ChrisPenner
Copy link
Member

@ChrisPenner ChrisPenner commented Dec 7, 2023

Overview

Fixes #4277

The local ui would often show the docs for the wrong term if the term-name was a valid suffix of some other definition in your lib (or elsewhere in the project).

Now it doesn't 🎉

Implementation notes

Adds the requirement to specify what kind of search you want when looking up HQ names from a NamesWithHistory. It used to ALWAYS do a suffix search, but sometimes you know exactly what you want 👍🏼

Share already has the correct behaviour here, but here's the fix for Local.

Test coverage

Tested the reproduction from #4277 against both and it now behaves correctly when:

  • local definition doesn't have a doc, but the lib definition does (doesn't show any doc)
  • local definition has a doc (shows that doc)

Controversial opinions

It's slightly more annoying to specify what kind of search you want in all these spots, but I think we'll see even more bugs like this pop up unless we make the caller choose.

Side benefit that it's more performant to look up by the exact name if you know you have it.

Loose ends

None

@ChrisPenner ChrisPenner self-assigned this Dec 7, 2023
@ChrisPenner ChrisPenner marked this pull request as ready for review December 7, 2023 23:19
@ChrisPenner ChrisPenner requested a review from aryairani December 7, 2023 23:19
@aryairani
Copy link
Contributor

Would you go into more detail about the controversial opinion? What would not having to specify look like?

@ChrisPenner
Copy link
Member Author

@aryairani The current code doesn't allow you to specify, but of course also gives incorrect results in these cases.

Other options would be to not use a name search at all in those cases, or to add a completely separate method to the name search, but I still think that forcing people to make the choice about their search type is going to result in the fewest bugs long term.

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.

UI shows docs for other FQN
2 participants