Skip to content

Optional method params go in method's options type#124

Merged
jhendrixMSFT merged 1 commit intomainfrom
json-merge-patch
Oct 21, 2024
Merged

Optional method params go in method's options type#124
jhendrixMSFT merged 1 commit intomainfrom
json-merge-patch

Conversation

@jhendrixMSFT
Copy link
Member

Emit associated builder methods for method options. Sort fields in method options types.
Include RequestContent as applicable to method options. Include application/merge-patch+json for serde format. Added cadl-ranch json-merge-patch scenario and tests. Note that some tests currently fail due to lack of support for sending JSON nulls. Added latest blob storage tsp.

@heaths
Copy link
Member

heaths commented Oct 16, 2024

Added cadl-ranch json-merge-patch scenario and tests. Note that some tests currently fail due to lack of support for sending JSON nulls. Added latest blob storage tsp.

Could we keep these as TODOs for now? We don't need them and I hate to jump the gun before we decide on Azure/azure-sdk-for-rust#1649.

Emit associated builder methods for method options.
Sort fields in method options types.
Include RequestContent<T> as applicable to method options.
Include application/merge-patch+json for serde format.
Added cadl-ranch json-merge-patch scenario and tests. Note that some
tests currently fail due to lack of support for sending JSON nulls.
Added latest blob storage tsp.
Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

A few thoughts but nothing blocking.

rust-version.workspace = true

[dependencies]
async-std = { workspace = true }
Copy link
Member

Choose a reason for hiding this comment

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

Is this only for tests? It should be in dev-dependencies then. We don't want to be taking a hard dependency on any async runtime. Despite its name, this is not a "std" library and its future is uncertain.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will fix this up when I switch tests to tokio::test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tracked as part of #127.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh I forgot this is more than just a test dependency. We use async_std::task::block_on when implementing TryFrom<Response<T>> on model types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants