Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Clean up #8536

Merged
merged 3 commits into from
Mar 20, 2023
Merged

Clean up #8536

merged 3 commits into from
Mar 20, 2023

Conversation

RussKie
Copy link
Member

@RussKie RussKie commented Mar 20, 2023

@codecov
Copy link

codecov bot commented Mar 20, 2023 •
edited
Loading

Codecov Report

Patch coverage: 77.77% and project coverage change: -0.01 ⚠️

Comparison is base (a3aeda1) 76.29% compared to head (50d8f94) 76.29%.

❗ Current head 50d8f94 differs from pull request most recent head b7f52f5. Consider uploading reports for the commit b7f52f5 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #8536      +/-   ##
==========================================
- Coverage   76.29%   76.29%   -0.01%     
==========================================
  Files         858      858              
  Lines       30639    30638       -1     
==========================================
- Hits        23377    23375       -2     
- Misses       7262     7263       +1
Impacted Files Coverage Δ
...icrosoft.DocAsCode.Build.Engine/DocumentBuilder.cs 75.12% <ø> (-0.13%) ⬇️
...rosoft.DocAsCode.Dotnet/SymbolFormatter.Symbols.cs 21.50% <0.00%> (ø)
....DocAsCode.Dotnet/Visitors/SymbolVisitorAdapter.cs 94.55% <100.00%> (ø)
...ft.DocAsCode.Dotnet/Visitors/YamlModelGenerator.cs 97.87% <100.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

yufeih
yufeih approved these changes Mar 20, 2023
View reviewed changes
Copy link
Contributor

@yufeih yufeih left a comment

Choose a reason for hiding this comment

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

Thank you @RussKie !

samples/Directory.Build.props Outdated Show resolved Hide resolved
@yufeih yufeih added the engineering Makes the pull request to appear in the "Engineering" section of the next release note label Mar 20, 2023
@RussKie
Consolidate common props in root Directory.Build.props
152886c
@RussKie
Use CPM
2110aaa 
See https://learn.microsoft.com/nuget/consume-packages/Central-Package-Management
@RussKie
Resolve warnings
b7f52f5 
* Suppress obsolete warnings CS0612 and CS0618

* Suppress NU1701, see https://learn.microsoft.com/nuget/reference/errors-and-warnings/nu1701

* Remove unused variables

* Resolves RS1024: Use 'SymbolEqualityComparer' when comparing symbols
@yufeih yufeih enabled auto-merge (squash) March 20, 2023 11:06
@yufeih yufeih disabled auto-merge March 20, 2023 11:07
@yufeih yufeih enabled auto-merge (squash) March 20, 2023 11:07
@yufeih yufeih merged commit 7e12c2c into dotnet:main Mar 20, 2023
@RussKie RussKie deleted the cleanup branch March 20, 2023 21:27
WeihanLi
WeihanLi reviewed Mar 21, 2023
View reviewed changes
Directory.Packages.props
<PackageVersion Include="Microsoft.CodeAnalysis.VisualBasic" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.Workspaces.Common" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.Workspaces.MSBuild" Version="4.5.0" />
<PackageVersion Include="Microsoft.SourceLink.GitHub" Version="1.1.1" />
Copy link
Contributor

@WeihanLi WeihanLi Mar 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe this can be specified as a GlobalPackageReference

https://learn.microsoft.com/en-us/nuget/consume-packages/central-package-management#global-package-references

Copy link
Contributor

@yufeih yufeih Mar 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Aha, I don't have a strong opinion, with a slight preference of NOT using central package version management because I can just copy and paste from NuGet.org and let @dependabot take care of the rest😁

Copy link
Member Author

@RussKie RussKie Mar 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I actually never seen that property. Though, according to the docs it's appropriate for references that apply to all projects. Not sure, if any of these truly do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good from my side, I'm using this in my project and the NuGet package has source link info also https://www.nuget.info/packages/dotnet-execute/0.13.0.

image

@ZhongRuoyu ZhongRuoyu mentioned this pull request Mar 21, 2023
Closed
@BrewTestBot BrewTestBot mentioned this pull request Mar 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WeihanLi WeihanLi WeihanLi left review comments

@yufeih yufeih yufeih approved these changes

Assignees
No one assigned
Labels
engineering Makes the pull request to appear in the "Engineering" section of the next release note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RussKie @yufeih @WeihanLi