Conversation
There was a problem hiding this comment.
the = is not aligned with the rest of the code. My recommendation is to shift the rest equal signs right wards to align with this one.
There was a problem hiding this comment.
It's either allowing exceptions or updating each line in the file when a new constant exceeds the allotted line. I prefer exceptions.
There was a problem hiding this comment.
Hi Aliaksei, what is your reason to make it an exception.
--Gong
-----Original Message-----
From: Aliaksei Baturytski [mailto:reply@reply.github.com]
Sent: Monday, March 19, 2012 9:18 AM
To: Albert Cheng
Subject: Re: [azure-sdk-for-net] Messaging, part II. (#13)
@@ -32,7 +32,9 @@ internal static class Constants
internal const string MessageDestination = "{0}/messages/"; internal const string UnlockedMessagePath = "{0}/messages/head?timeout={1}";
internal const string UnlockedSubscriptionMessagePath = "{0}/subscriptions/{1}/messages/head?timeout={2}";
It's either allowing exceptions or updating each line in the file when a new constant exceeds the allotted line. I prefer exceptions.
Reply to this email directly or view it on GitHub:
https://github.com/WindowsAzure/azure-sdk-for-net/pull/13/files#r574869
There was a problem hiding this comment.
For now this is OK until we get StyleCop working together. I believe in general we avoid spacing statements this way, but it doe slook clean for constant strings. No need to change in my mind for now.
There was a problem hiding this comment.
My reason was to minimize # of lines I am chaning - this pull request is big enough even already. Also, unless there's a restriction on length of constants/variables, you can never predict how much space you should allow between a variable and a value. I personally think that such formatting is definitely nice to have, but it should not be required because in case of long variable names it actually reduces readability of the code instead of improving it.
There was a problem hiding this comment.
Is there any follow-up required here, creating issues or filing a bug on the server, or can the code just be removed? OK to stay if it needs to be there for consistency.
There was a problem hiding this comment.
I think it will be removed. I'll replace it with a plain comment.
Some implementation details you may have questions about:
Public constructors in BrokeredMessageSettings class have been replaced with the static methods because JavaScript cannot handle multiple constructors with the same number of parameters.
Message's property bag is populated with all headers. I did this for compatibility wiht Java SDK, which does the same thing.