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.

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

Align whole comments between netfx and netcore #2955

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

MichelZ
Copy link
Contributor

@MichelZ MichelZ commented Nov 2, 2024

All this PR does is deal with comments, so hopefully easy to review
Mostly removes obsolete comments from netfx to align netfx and netcore for later mering

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.

To be honest I'm not sure about this one. I appreciate the efforts to make merging this class easier, but I'm also concerned about losing in-code documentation.

Questions for other reviewers:

  • Are these TODOs actually done?
  • What does tagging something as UNDONE actually mean?
  • Is there any benefit to keeping old ticket IDs? I personally wouldn't even know where to go looking for them.

Would appreciate review from @ErikEJ or @Wraith2 before I approve

@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 4, 2024

I‘m also happy to switch it around to add the info on the other side, if that‘s the concern.

AFAIK these things were removed on the netcore side before for a reason I guess

@ErikEJ
Copy link
Contributor

ErikEJ commented Nov 4, 2024

I vote for keeping them for the purpose of merging.

@benrr101
Copy link
Contributor

benrr101 commented Nov 5, 2024

@ErikEJ Sorry, I'm a bit confused with that answer. Are you saying "leave the old ticket IDs until merging is completed then remove them", "leave old ticket IDs indefinitely", or something else?

Also what does UNDONE mean in these contexts? @cheenamalhotra @saurabh500

@benrr101
Copy link
Contributor

benrr101 commented Nov 5, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ErikEJ
Copy link
Contributor

ErikEJ commented Nov 5, 2024

@benrr101 postpone the decision to remove them (or not) ...

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 72.72727% with 6 lines in your changes missing coverage. Please review.

Project coverage is 72.43%. Comparing base (4bc9ee6) to head (9f984ce).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 57.14% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2955      +/-   ##
==========================================
- Coverage   72.50%   72.43%   -0.07%     
==========================================
  Files         288      288              
  Lines       59529    60229     +700     
==========================================
+ Hits        43159    43629     +470     
- Misses      16370    16600     +230     
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 75.38% <100.00%> (-0.06%) ⬇️
netfx 70.93% <57.14%> (+<0.01%) ⬆️

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

@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.

As per the suggestion from @ErikEJ, let's retain the old ticket information and todo/undone tags. I think that more/less amounts to copying them from netfx to netcore. Although I personally like removing stuff, I don't want to lose any history if possible.

Thanks for your patience on this one 👍

@MichelZ
Copy link
Contributor Author

MichelZ commented Nov 6, 2024

OK, no worries. Will adjust

@MichelZ MichelZ closed this Nov 6, 2024
@MichelZ MichelZ force-pushed the merge-tdsparser-removecomments branch from 3a0d101 to 9d5ca32 Compare November 6, 2024 18:40
@MichelZ
Copy link
Contributor Author

MichelZ commented Nov 6, 2024

@benrr101 Done

@benrr101
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@cheenamalhotra cheenamalhotra merged commit 737db36 into dotnet:main Nov 13, 2024
76 checks passed
@cheenamalhotra cheenamalhotra added this to the 6.0-preview3 milestone Nov 13, 2024
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