Skip to content

Conversation

MichelZ
Copy link
Contributor

@MichelZ MichelZ commented Nov 2, 2024

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

Part of #2953

Copy link
Contributor

@benrr101 benrr101 left a comment

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 😅

@benrr101 benrr101 added the Common Project 🚮 Things that relate to the common project project label Nov 4, 2024
@MichelZ
Copy link
Contributor Author

MichelZ commented Nov 5, 2024

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
Contributor

benrr101 commented Nov 5, 2024

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
Contributor

benrr101 commented Nov 5, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link

codecov bot commented Nov 6, 2024

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 lines Patch % Lines
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 87.50% 8 Missing ⚠️
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 0.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     
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 75.80% <0.00%> (?)
netfx 71.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.

Copy link
Contributor

@mdaigle mdaigle left a comment

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
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichelZ
Copy link
Contributor Author

MichelZ commented Nov 11, 2024

AZP needs a smack on the head

@MichelZ MichelZ force-pushed the merge-tdsparser-typealiases branch from f335f1b to 36fd740 Compare November 13, 2024 08:47
@MichelZ
Copy link
Contributor Author

MichelZ commented Nov 19, 2024

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

@MichelZ MichelZ changed the title Align Type aliases between netfx and netcore Merge | Align Type aliases between netfx and netcore Nov 24, 2024
@benrr101 benrr101 dismissed cheenamalhotra’s stale review November 25, 2024 22:59

Requested change has been addressed

@benrr101 benrr101 added this to the 6.0.0 milestone Nov 25, 2024
@benrr101 benrr101 merged commit 16890fb into dotnet:main Nov 25, 2024
76 checks passed
@cheenamalhotra cheenamalhotra modified the milestones: 6.0.0, 6.1-preview1 Apr 30, 2025
Sign up for free to 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