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.

Sign up for GitHub

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

Jump to bottom

Add support for parameters of type TimeSpan to SqlFilter #7325

Merged
merged 1 commit into from
Aug 21, 2019
Merged

Add support for parameters of type TimeSpan to SqlFilter #7325

merged 1 commit into from
Aug 21, 2019

Conversation

SeanFeldman
Copy link
Contributor

Fixes #6629

@SeanFeldman
Fix #6629
f2e108a 
.NET standard library does not allow using parameters of type TimeSpan
@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

jsquire
jsquire reviewed Aug 19, 2019
View reviewed changes
Copy link
Member

@jsquire jsquire left a comment

Choose a reason for hiding this comment

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

Implementation looks good to me. Since I lack a deeper context and understanding of how these changes may fit into the stack, I'm going to ask that @nemakam and @binzywu take ownership of approval here.

sdk/servicebus/Microsoft.Azure.ServiceBus/src/Management/ManagementClientConstants.cs
@@ -16,6 +16,7 @@ internal class ManagementClientConstants
public const string SbNs = "http://schemas.microsoft.com/netservices/2010/10/servicebus/connect";
public const string XmlSchemaInstanceNs = "http://www.w3.org/2001/XMLSchema-instance";
public const string XmlSchemaNs = "http://www.w3.org/2001/XMLSchema";
public const string SerializationNamespace = "http://schemas.microsoft.com/2003/10/Serialization/";
Copy link
Member

Choose a reason for hiding this comment

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

nit: The other members of this file use Ns where this uses Namespace; maybe we should consider unifying on one representation? Personally, my preference goes to Namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm good with either @jsquire. Fully spelled out version is easier to read though. So if @nemakam agrees, I'll update the other two constant names despite being out of the context.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am good with either.
@SeanFeldman, you can take the call to go either way :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be addressed in #7399

@jsquire jsquire assigned jsquire and nemakam and unassigned jsquire Aug 19, 2019
@jsquire jsquire requested a review from binzywu August 19, 2019 15:12
@jsquire jsquire added Client This issue points to a problem in the data-plane of the library. Service Bus labels Aug 19, 2019
@jsquire jsquire requested a review from bainian12345 August 19, 2019 15:13
@nemakam
Copy link
Contributor

nemakam commented Aug 20, 2019

@SeanFeldman, The changes look good to me. Let me know whenever you are ready to merge, and I can merge it in!

@axisc
Copy link

axisc commented Aug 21, 2019

@jsquire , Can you merge the changes?

@nemakam has approved.

@jsquire jsquire merged commit ebb6f0b into Azure:master Aug 21, 2019
@SeanFeldman SeanFeldman deleted the support-for-timespan branch August 21, 2019 22:53
@SeanFeldman
Copy link
Contributor Author

@axisc I guess you and @jsquire didn't read through #7325 (comment) ... 🙂

I'll issue a separate PR.
Lesson: when a PM says merge, don't rush 😹

@jsquire
Copy link
Member

jsquire commented Aug 22, 2019

@jsquire didn't read through #7325 (comment) ... 🙂

Truth. I'm just making up for permissions still not quite being right in the repo by blindly pushing buttons on request. Though, I forgot that Neeraj is empowered now and should have deferred. Probably not the biggest mistake that I made yesterday.

@SeanFeldman SeanFeldman mentioned this pull request Aug 26, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsquire jsquire jsquire left review comments

@nemakam nemakam nemakam approved these changes

@erich-wang erich-wang Awaiting requested review from erich-wang

@binzywu binzywu Awaiting requested review from binzywu

@bainian12345 bainian12345 Awaiting requested review from bainian12345

Assignees

@nemakam nemakam

Labels
Client This issue points to a problem in the data-plane of the library. Service Bus
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

.net standard library does not allow using parameters of type TimeSpan
5 participants
@SeanFeldman @azuresdkci @nemakam @axisc @jsquire