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

[UIKit] Correct nullability attributes for parts of UIKit #15200

Merged
merged 8 commits into from
Jun 8, 2022
Merged

[UIKit] Correct nullability attributes for parts of UIKit #15200

merged 8 commits into from
Jun 8, 2022

Conversation

rpendleton
Copy link
Contributor

@rpendleton rpendleton commented Jun 3, 2022

These changes correct some of the nullability attributes for the UIKit bindings. Although UIKit has several hundred lines of xtro-sharpie ignores related to nullability, this PR only fixes a few of them.

I wasn't sure where to draw the line, but in the end, I essentially fixed most of UIViewController and all of UITableView and UICollectionView. These classes were the ones that led to the most // ReSharper disable comments being added in my team's codebase, and/or that led to crashes being introduced by devs following ReSharper's suggestions to remove "unnecessary" null-checks.

These changes were largely based on the instructions given by @spouliot in #6154. However, I noticed the xtro-sharpie ignore files have been duplicated in the api-annotations-dotnet directory since then, so I updated the .ignore and .todo files in that directory as well.

If the process has changed since that comment was written, I can make any additional changes that are needed.

@rolfbjarne rolfbjarne added the community Community contribution ❤ label Jun 3, 2022
@rolfbjarne
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

Copy link
Member

@rolfbjarne rolfbjarne left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks!

@rolfbjarne
Copy link
Member

These changes were largely based on the instructions given by @spouliot in #6154. However, I noticed the xtro-sharpie ignore files have been duplicated in the api-annotations-dotnet directory since then, so I updated the .ignore and .todo files in that directory as well.

That's correct, since spouliot wrote those instructions, we have a different set if xtro files for the .NET assemblies, and you'll have to remove the same entries from those files as well. I've updated the instructions in that comment accordingly.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@chamons
Copy link
Contributor

chamons commented Jun 6, 2022

Looks like some of the CI choked on the build:

##[error]We stopped hearing from agent VSM-XAM-35.BigSur.arm64. Verify the agent machine is running and has a healthy network connection. Anything that terminates an agent process, starves it for CPU, or blocks its network access can cause this error. For more information, see: https://go.microsoft.com/fwlink/?linkid=846610

@mandel-macaque - Can this go in? I think that's signing stuff and this is fine, but I'm out of the loop here.

@mandel-macaque
Copy link
Member

Results seem ok, but I have merge with main and will re-trigger a build.

@mandel-macaque
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@rolfbjarne
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vs-mobiletools-engineering-service2
Copy link
Collaborator

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1107.Monterey'
Hash: a8370e9297f7ee7dc7643c4d01275dd4d6159a26

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [PR Build] Tests on macOS Mac Catalina (10.15) failed ❌

Failed tests are:

  • monotouch-test

Pipeline on Agent
Hash: a8370e9297f7ee7dc7643c4d01275dd4d6159a26

@vs-mobiletools-engineering-service2
Copy link
Collaborator

📋 [PR Build] API Diff 📋

API diff (for current PR)

ℹ️ API Diff (from PR only) (please review changes)

API diff: vsdrops gist

Xamarin
.NET
Xamarin vs .NET
iOS vs Mac Catalyst (.NET)

API diff (vs stable)

✅ API Diff from stable

API diff: vsdrops gist

Xamarin
.NET
Xamarin vs .NET
iOS vs Mac Catalyst (.NET)

Generator diff

ℹ️ Generator Diff (please review changes)

Pipeline on Agent XAMBOT-1109.Monterey
Hash: a8370e9297f7ee7dc7643c4d01275dd4d6159a26

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻

All tests on macOS M1 - Mac Big Sur (11.5) passed.

Pipeline on Agent
Hash: a8370e9297f7ee7dc7643c4d01275dd4d6159a26

@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ [CI Build] Tests passed on VSTS: simulator tests iOS. ✅

Tests passed on VSTS: simulator tests iOS.

🎉 All 148 tests passed 🎉

Pipeline on Agent XAMBOT-1030.Monterey'
Merge a8370e9 into 6c5a95b

@rolfbjarne rolfbjarne merged commit 4924934 into xamarin:main Jun 8, 2022
@rolfbjarne
Copy link
Member

Great work! Thanks a lot!

@rpendleton rpendleton deleted the improvement/FixUIKitNullability branch October 14, 2022 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution ❤
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants