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

Conversation

@sqs
Copy link
Member

@sqs sqs commented Dec 5, 2018

I made these fixes while working on sourcegraph/code-intel-extensions#8.

Fixes #838

@codecov-io
Copy link

codecov-io commented Dec 5, 2018

Codecov Report

Merging #1240 into master will decrease coverage by 0.01%.
The diff coverage is 71.42%.

Impacted Files Coverage Δ
shared/src/api/client/context/contextService.ts 100% <ø> (ø) ⬆️
...ared/src/panel/views/HierarchicalLocationsView.tsx 0% <ø> (ø) ⬆️
shared/src/components/LinkOrButton.tsx 20% <ø> (ø) ⬆️
shared/src/actions/ActionItem.tsx 12.69% <0%> (-0.21%) ⬇️
shared/src/api/extension/api/configuration.ts 87.5% <100%> (ø) ⬆️
shared/src/api/client/services/location.ts 77.77% <100%> (+2.16%) ⬆️
shared/src/commands/commands.ts 32.55% <60%> (-4.95%) ⬇️

GROUPS.map(
({ name, defaultSize }, i) =>
((groupByFile && name === 'file') || groups[i].length > 1) && (
({ name, defaultSize, key }, i) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is getting pretty hard to parse, and the reasoning behind all the cases you're checking for isn't clear. I suggest extracting this logic and making it more explicit, eg:

const groupsToDisplay = GROUPS.filter(({ name, key }, i) => {
    // No locations were returned for this group - don't display it
    if (!groups[i]) {
        return false
    }
    // Always display file groups
    else if (groupByFile && name === 'file') {
        return true
    }
    ...
})

return (
    <div className={`hierarchical-locations-view ${this.props.className || ''}`}>
        {selectedGroups && groupsToDisplay.map(({ name, defaultSize }, i) => {
            ...
         })}

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. I will make this code much clearer.

@sqs sqs force-pushed the basic-code-intel branch from 62e4657 to d6e860b Compare December 6, 2018 04:55
@sqs sqs force-pushed the basic-code-intel branch from d6e860b to b7e2a2d Compare December 6, 2018 05:16
@sqs sqs force-pushed the basic-code-intel branch from 04ddc15 to e27bc9c Compare December 6, 2018 05:43
@sqs
Copy link
Member Author

sqs commented Dec 6, 2018

@lguychard Thanks for the review! Updated, PTAL.

Copy link
Contributor

@lguychard lguychard left a comment

Choose a reason for hiding this comment

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

LGTM

@sqs sqs force-pushed the basic-code-intel branch from d94cd58 to 2d95059 Compare December 6, 2018 06:36
@sqs sqs force-pushed the basic-code-intel branch from 2d95059 to 3fea559 Compare December 6, 2018 06:38
@sqs sqs merged commit 3fea559 into master Dec 6, 2018
@sqs sqs deleted the basic-code-intel branch December 6, 2018 06:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error in references breaks subsequent hovers

4 participants