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

[healthkit] Add nullability to (generated and manual) bindings #14730

Merged
merged 12 commits into from
Apr 28, 2022

Conversation

tj-devel709
Copy link
Member

This PR aims to bring nullability changes to HealthKit.
Following the steps here:

  1. I am looking for any !missing-null-allowed! in this frameworks ignore files and applying them
  2. I am adding nullable enable to all manual files that are not "API_SOURCES" in src/frameworks.sources and making the required nullability changes
  3. Changing all throw new ArgumentNullException ("object")); to ObjCRuntime.ThrowHelper.ThrowArgumentNullException (nameof (object)); for size saving optimization as well to mark that this framework contains nullability changes
  4. Changing any == null or != null to is null and is not null

@tj-devel709 tj-devel709 added the not-notes-worthy Ignore for release notes label Apr 12, 2022
@tj-devel709 tj-devel709 added this to the Future milestone Apr 12, 2022
public class HKHealthStoreTest {

[Test]
public void GetBiologicalSexNullReturnTest ()
Copy link
Member Author

Choose a reason for hiding this comment

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

These tests are here because the docs said the returns were not nullable, but the headers did say they were. The tests show that they are in fact nullable

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ Tests on macOS Mac Catalina (10.15) passed ✅

Tests passed

All tests on macOS Mac Catalina (10.15) passed.

Pipeline on Agent
Merge fc907e3 into b8eb1c3

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ Tests on macOS M1 - Mac Big Sur (11.5) failed ❌

Failed tests are:

  • linksdk

Pipeline on Agent
Merge fc907e3 into b8eb1c3

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

Looks good once Manuel's comments are addressed.

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

Copy link
Member

@mandel-macaque mandel-macaque left a comment

Choose a reason for hiding this comment

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

You have added tests for HKHealthStore which is AFAIK not present in mac os x. So please confirm that the new test is the reason for the mac tests to fail.

@vs-mobiletools-engineering-service2
Copy link
Collaborator

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1098.Monterey
Hash: de37f9359c26d10c4800b4d72137bb91e0654072

@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: de37f9359c26d10c4800b4d72137bb91e0654072

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻

All tests on macOS Mac Catalina (10.15) passed.

Pipeline on Agent
Hash: de37f9359c26d10c4800b4d72137bb91e0654072

@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-1044.Monterey'
Merge de37f93 into 46a8971

@tj-devel709 tj-devel709 merged commit 6dc0eed into dotnet:main Apr 28, 2022
@mandel-macaque
Copy link
Member

This is a review comment from GraphQL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-notes-worthy Ignore for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants