-
Notifications
You must be signed in to change notification settings - Fork 684
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add release notes #438
Add release notes #438
Conversation
/// The package's release notes. | ||
/// </summary> | ||
[JsonProperty("releaseNotes")] | ||
public string ReleaseNotes { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the properties on PackageMetadata
are part of the official NuGet protocol (see this).
Please move this to BaGetPackageMetadata
, as that is where all of BaGet's custom additions to the protocol are placed. See:
/// BaGet's extensions to the package metadata model. These additions |
Hi, thanks for opening this pull request! Would you mind also updating the Azure Table package service? These places will need to be updated:
BaGet/src/BaGet.Azure/Table/PackageEntity.cs Lines 12 to 46 in 05bfc61
BaGet/src/BaGet.Azure/Table/TableOperationBuilder.cs Lines 20 to 50 in 05bfc61
BaGet/src/BaGet.Azure/Table/TablePackageService.cs Lines 223 to 251 in 05bfc61
Very nice work! :) |
Thanks for the review. Will update according to commets. I wanted to ask you one thing. I deployed this version of Baget and replaced it with the previous release. The issue is that the Release notes did not get updated. I could see the Section in the UI, but without the actual notes. For new packages release notes are updated properly. What extra would I need to do so that while upgrading the release notes for old packages are also reflected. |
Sadly today there’s no way to “reprocess” already uploaded packages. I opened an issue to track this: #447 Today, you’ll need to:
|
Your changes look good to me, I’ll test this when I’m back home and then merge this in! |
/// The package's release notes. | ||
/// </summary> | ||
[JsonProperty("releaseNotes")] | ||
public string ReleaseNotes { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this to after PackageTypes
but before RepositoryUrl
to keep these properties alphabetically sorted?
@@ -247,6 +248,10 @@ class DisplayPackage extends React.Component<IDisplayPackageProps, IDisplayPacka | |||
<Dependents packageId={this.state.package.id} /> | |||
</ExpandableSection> | |||
|
|||
<ExpandableSection title="Release Notes" expanded={false}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most packages don't have release notes. Let's hide the section if the package is missing release notes:
{this.state.package.releaseNotes &&
<ExpandableSection title="Release Notes" expanded={false}>
<div className="package-release-notes" >{this.state.package.releaseNotes}</div>
</ExpandableSection>
}
Ok I tested this and it worked like a charm! I left two more minor comments, I'll merge this in once you've addressed them. Thanks for all the help! |
Thank you @cw-pratik for the excellent pull request! |
Summary of the changes (in less than 80 chars)