Skip to content

GitHub Integration (Proof of Concept)#578

Draft
davidackroyd99 wants to merge 6 commits into
loic-sharma:mainfrom
davidackroyd99:feature/da/github-integration
Draft

GitHub Integration (Proof of Concept)#578
davidackroyd99 wants to merge 6 commits into
loic-sharma:mainfrom
davidackroyd99:feature/da/github-integration

Conversation

@davidackroyd99

Copy link
Copy Markdown
Contributor

I did this, but the GitHub API often seems to return incorrect results for open issues (whether you use 'open_issues' or 'open_issues_count'), and doesn't return number of open PRs at all. Would we not be better off listing the number of stars and forks?

Once we have decided on stats and so on to return I will clean up the code, but this works as a proof of concept.

Addresses #97

@loic-sharma

Copy link
Copy Markdown
Owner

!!!! This is awesome. I'll review this in a few hours when I'm off work

@loic-sharma

loic-sharma commented Aug 25, 2020

Copy link
Copy Markdown
Owner

the GitHub API often seems to return incorrect results for open issues (whether you use 'open_issues' or 'open_issues_count'), and doesn't return number of open PRs at all. Would we not be better off listing the number of stars and forks?

I think showing the number of stars and forks is a great start. Personally I'm not as interested in number of issues or open PRs.

<p>{(!this.state.package.authors) ? 'Unknown' : this.state.package.authors}</p>
</div>

<RepoDetails url={this.state.package.projectUrl}/>

@loic-sharma loic-sharma Aug 25, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The projectUrl is the package's website, not necessarily it's source repository. Please use the repositoryUrl property instead:

[JsonPropertyName("repositoryUrl")]
public string RepositoryUrl { get; set; }

This property isn't part of the NuGet V3 protocol, but, it maps to the package's source repository metadata: https://docs.microsoft.com/en-us/nuget/reference/nuspec#repository

By the way, the repositoryUrl property on the TypeScript interface should be nullable. Could you fix that as part of your changes?

repositoryUrl?: string;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will do!

detailsFound: boolean;
}

class RepoDetails extends React.Component<IRepoDetailsProps, IRepoDetailsState> {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Just curious, why did you create a new component instead of updating the SourceRepository component?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Once I've finished this I might remove the heading tag and put it inside the SourceRepository component, this is a good point

Comment thread src/BaGet.UI/src/DisplayPackage/RepoDetails.tsx
Comment thread src/BaGet.UI/src/DisplayPackage/RepoDetails.tsx
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.

2 participants