Skip to content
This repository was archived by the owner on Mar 18, 2024. It is now read-only.

Conversation

@efritz
Copy link
Contributor

@efritz efritz commented Feb 3, 2021

Before:

Screen Shot 2021-02-03 at 2 38 02 PM

After:

Screen Shot 2021-02-03 at 2 37 41 PM

Summary:

Each supported language in these extensions have a set of properties attached to them (file extensions, what identifier patterns look like, what comments look like, etc). One of these properties is docstringIgnore, which will allow us to ignore the lines between where a ctags definition occurs and the comment that is meant to belong to it. Until now, all we've used this for is for Java-style annotations, e.g.

/** docstring **/
@MyBigAnnotation
class Something {
    // ...
}

This adds support for the C-family convention of breaking definitions up onto multiple lines:

/** docstring **/
RET_VALUE
ActualMethod(...) {
    // ...
}

This could have unintended side-effects where comments that do not belong to a ctags definition just happens to be close enough to it, but I think it's an acceptable risk tradeoff (while hand-testing I couldn't get it to do something worse than the previous behavior).

Fixes https://github.com/sourcegraph/sourcegraph/issues/17925.

@efritz efritz added the team/code-intelligence Code intelligence team label Feb 3, 2021
@efritz efritz added this to the Code Intelligence Sprint 5 milestone Feb 3, 2021
@efritz efritz requested review from a team, Strum355, garobrik and tjdevries February 3, 2021 20:44
@efritz efritz self-assigned this Feb 3, 2021
@efritz efritz changed the title Ef/cpp docstring ignore Expand C-family docstring resolution Feb 3, 2021
@tjdevries
Copy link

Side question: How to add a test for something like this (is it simple?)

@efritz
Copy link
Contributor Author

efritz commented Feb 3, 2021

@tjdevries It's difficult to test changes to the language spec properties, so we default back to testing by hand. In order to run the extensions, you can side load it against a live SG instance (I used sourcegraph.com here as a user reported an actual line of code, which is always nice).

(Note that I had to disable the default sourcegraph/cpp instance in my user settings so I didn't have two extensions fighting over each other to provide slightly different data).

It would be nice to add integration tests here, but it could be a bit heavy-handed as it requires an instance (and search results from the backend). There are a good amount of tests in the machinery though, so I'm confident we didn't regress on things outside of the C-family heuristics that were changed here.

We have tests to ensure we are using the regex pattern correctly, we just don't test the explicit pattern on the language which is hopefully declarative and trivial.

Copy link

@garobrik garobrik left a comment

Choose a reason for hiding this comment

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

LGTM

@efritz efritz merged commit b72b4ed into master Feb 3, 2021
@efritz efritz deleted the ef/cpp-docstring-ignore branch February 3, 2021 22:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

team/code-intelligence Code intelligence team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Search-based code intel hovers are broken when definition token and docstring are separated by multiple lines

4 participants