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

Enabling NRT for SWF (no API changes yet) #2702

Merged
merged 2 commits into from Jan 23, 2020
Merged

Enabling NRT for SWF (no API changes yet) #2702

merged 2 commits into from Jan 23, 2020

Conversation

@Logerfo
Copy link
Contributor

Logerfo commented Jan 13, 2020

Work for #1107.
Refers to #2678.
Relates to #2676.


Changes

  • Added <Nullable>enable</Nullable> to System.Windows.Forms.csproj.
  • Added #nullable disable to every src/System.Windows.Forms/src/**/*.cs file header.

Next steps

  • First we have to make sure this PR implies no API changes. (#2705)
  • After merging this one, then multiple PRs can be issued annotating few scoped and targeted API changes at a time.

cc @RussKie @sharwell @hughbe

Microsoft Reviewers: Open in CodeFlow
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 13, 2020

Codecov Report

Merging #2702 into master will increase coverage by 0.01189%.
The diff coverage is n/a.

@@                Coverage Diff                 @@
##              master       #2702        +/-   ##
==================================================
+ Coverage   58.88815%   58.90005%   +0.0119%     
==================================================
  Files           1242        1242                
  Lines         428565      428565                
  Branches       38912       38912                
==================================================
+ Hits          252374      252425        +51     
+ Misses        170849      170796        -53     
- Partials        5342        5344         +2
Flag Coverage Δ
#Debug 58.90005% <ø> (+0.01189%) ⬆️
#production 31.83119% <ø> (+0.01993%) ⬆️
#test 98.97497% <ø> (ø) ⬆️
@Logerfo

This comment has been minimized.

Copy link
Contributor Author

Logerfo commented Jan 13, 2020

I just did a full eye-check on the API and found a couple of changes: one for a file I had considered as safe and the other one for a file I forgot to edit.
I don't think their changes were relevant (only [NullableContext(1)][Nullable(0)] at class site), but "reverted" them anyway, just for the sake of it.
I'd call the PR API-changeless, but I'd rather be sure with some sort of currently lacking automated API-diffing.

@RussKie

This comment has been minimized.

Copy link
Member

RussKie commented Jan 14, 2020

I will need to get #2705 before this can go in.

@Logerfo

This comment has been minimized.

Copy link
Contributor Author

Logerfo commented Jan 14, 2020

@RussKie Oh, I didn't know about that txt files feature. That's exactly what I needed for the API diff. While rebasing, I removed that change from this branch and also filed #2708. After merged, this branch (and #2676) can be rebased again and the diagnosis will tell us if we have any undesired API changes (my local merge tells me this branch doesn't).

@Logerfo Logerfo force-pushed the Logerfo:nrt_swf branch from 8105557 to 53bac09 Jan 21, 2020
@Logerfo Logerfo changed the title WIP: Enabling NRT for SWF (no API changes yet) Enabling NRT for SWF (no API changes yet) Jan 21, 2020
@Logerfo Logerfo marked this pull request as ready for review Jan 21, 2020
@Logerfo Logerfo requested a review from dotnet/dotnet-winforms as a code owner Jan 21, 2020
@Logerfo

This comment has been minimized.

Copy link
Contributor Author

Logerfo commented Jan 21, 2020

@RussKie rebasing didn't throw any warning, I guess we're good to go.

@RussKie

This comment has been minimized.

Copy link
Member

RussKie commented Jan 22, 2020

Let's breaking it up in two:

  • the first PR adds #nullable disable (the commits 1 and 2), and
  • the second one - remove decorations where safe (the commits 3 and 4).

Right now commit 53bac09 undo changes made in dfd4820.
So squashing them together doesn't feel right, taking as is - we'll be merging in "draft changes" (the internal ways of you arriving to the final result).

@Logerfo Logerfo force-pushed the Logerfo:nrt_swf branch from 53bac09 to 5d93057 Jan 22, 2020
@Logerfo

This comment has been minimized.

Copy link
Contributor Author

Logerfo commented Jan 22, 2020

@RussKie done. This PR is acting as the first one you mentioned. I'm ready to send the other one once this one is merged.

@Logerfo Logerfo changed the title Enabling NRT for SWF (no API changes yet) WIP: Enabling NRT for SWF (no API changes yet) Jan 22, 2020
@Logerfo Logerfo force-pushed the Logerfo:nrt_swf branch from 5d93057 to 2f08412 Jan 22, 2020
@Logerfo

This comment has been minimized.

Copy link
Contributor Author

Logerfo commented Jan 22, 2020

Missed a new file from 9dc2a1d. It's an internal class, so it didn't show up in the API diff.

@Logerfo Logerfo changed the title WIP: Enabling NRT for SWF (no API changes yet) Enabling NRT for SWF (no API changes yet) Jan 22, 2020
@RussKie RussKie merged commit b666dc7 into dotnet:master Jan 23, 2020
5 checks passed
5 checks passed
WIP Ready for review
Details
dotnet-winforms CI Build #20200122.3 succeeded
Details
dotnet-winforms CI (Build Windows Debug) Build Windows Debug succeeded
Details
dotnet-winforms CI (Build Windows Release) Build Windows Release succeeded
Details
license/cla All CLA requirements met.
Details
@msftbot msftbot bot added this to the 5.0 milestone Jan 23, 2020
@RussKie

This comment has been minimized.

Copy link
Member

RussKie commented Jan 23, 2020

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.