Skip to content

Merge | Align Type aliases between netfx and netcore - #2957

Merged
benrr101 merged 5 commits into
dotnet:mainfrom
MichelZ:merge-tdsparser-typealiases
Nov 25, 2024
Merged

Merge | Align Type aliases between netfx and netcore#2957
benrr101 merged 5 commits into
dotnet:mainfrom
MichelZ:merge-tdsparser-typealiases

Conversation

@MichelZ

Copy link
Copy Markdown
Contributor

This PR solely aligns type aliases between netfx and netcore versions so they can be merged easier later.

Part of #2953

@MichelZMichelZ mentioned this pull request Nov 2, 2024

@benrr101benrr101 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, but going forward, I think we can combine a couple of PRs like this together. I'm starting to get fatigue from reviewing these all independently 😅

@benrr101benrr101 added the Common Project 🚮 Things that relate to the common project project label Nov 4, 2024
@MichelZ

Copy link
Copy Markdown
ContributorAuthor

Looks good, but going forward, I think we can combine a couple of PRs like this together. I'm starting to get fatigue from reviewing these all independently 😅

Apologies, I really thought it makes it easier for review to have smaller, more targeted PR's... Guess I was wrong :)
Will combine them in the future

@benrr101

Copy link
Copy Markdown
Contributor

There's definitely a balance :) I think we'll still take stuff when it's bite-sized like this, but it just might be slow with all the overhead. On the other hand, gigantic PRs are a lot more likely to just be outright rejected

@benrr101

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@codecov

codecovBot commented Nov 6, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 86.15385% with 9 lines in your changes missing coverage. Please review.

Project coverage is 73.18%. Comparing base (2151501) to head (36fd740).
Report is 33 commits behind head on main.

Files with missing linesPatch %Lines
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs87.50%8 Missing ⚠️
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs0.00%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #2957 +/- ##
===========================================
- Coverage 92.58% 73.18% -19.40% 
===========================================
Files 6 288 +282 Lines 310 65607 +65297 ===========================================
+ Hits 287 48014 +47727 - Misses 23 17593 +17570 
FlagCoverage Δ
addons92.58% <ø> (ø)
netcore75.80% <0.00%> (?)
netfx71.31% <87.50%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mdaiglemdaigle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good after merge conflicts are fixed

@benrr101

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@MichelZ

Copy link
Copy Markdown
ContributorAuthor

AZP needs a smack on the head

@MichelZ
MichelZforce-pushed the merge-tdsparser-typealiases branch from f335f1b to 36fd740CompareNovember 13, 2024 08:47
@MichelZ

Copy link
Copy Markdown
ContributorAuthor

@cheenamalhotra Anything I can do to move this along? Thx

@MichelZMichelZ changed the title Align Type aliases between netfx and netcoreMerge | Align Type aliases between netfx and netcoreNov 24, 2024
@benrr101
benrr101 dismissed cheenamalhotra’s stale reviewNovember 25, 2024 22:59

Requested change has been addressed

@benrr101benrr101 added this to the 6.0.0 milestone Nov 25, 2024
@benrr101
benrr101 merged commit 16890fb into dotnet:mainNov 25, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Common Project 🚮Things that relate to the common project project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@MichelZ@benrr101@mdaigle@cheenamalhotra@David-Engel