Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

gps: add new ProjectAnalyzerInfo type to return from ProjectAnalyzer.Info #753

Merged
merged 1 commit into from
Jun 15, 2017
Merged

gps: add new ProjectAnalyzerInfo type to return from ProjectAnalyzer.Info #753

merged 1 commit into from
Jun 15, 2017

Conversation

jmank88
Copy link
Collaborator

@jmank88 jmank88 commented Jun 14, 2017

What does this do / why do we need it?

This PR proposes a new simple struct type ProjectAnalyzerInfo to return from ProjectAnalyzer.Info(), and to use in place of ProjectAnalyzer in a few places which only called Info(). My working branch for #431 contains additional usages.

What should your reviewer look out for in this PR?

Better name or documentation opportunities. More usages of ProjectAnalyzer which only need ProjectAnalyzerInfo.

Which issue(s) does this PR fix?

Related #672 and #431.

Copy link
Member

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

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

at first this seemed like an unnecessary change, but it's vastly better to have known, static information keying the cache map than it is interface data. 👍

mostly this is good, just a nit on the docs.

Info() ProjectAnalyzerInfo
}

// ProjectAnalyzerInfo holds name and version information for a ProjectAnalyzer.
Copy link
Member

Choose a reason for hiding this comment

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

small modification: "ProjectAnalyzerInfo indicates a ProjectAnalyzer's name and version."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated.

@jmank88 jmank88 closed this Jun 14, 2017
@jmank88 jmank88 reopened this Jun 14, 2017
@sdboyer sdboyer merged commit 235878a into golang:master Jun 15, 2017
@jmank88 jmank88 deleted the project_analyzer_info branch June 15, 2017 00:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants