{"action":"created","issue":{"url":"https://api.github.com/repos/tarwn/BasicAzureStorageSDK/issues/11","labels_url":"https://api.github.com/repos/tarwn/BasicAzureStorageSDK/issues/11/labels{/name}","comments_url":"https://api.github.com/repos/tarwn/BasicAzureStorageSDK/issues/11/comments","events_url":"https://api.github.com/repos/tarwn/BasicAzureStorageSDK/issues/11/events","html_url":"https://github.com/tarwn/BasicAzureStorageSDK/pull/11","id":105869899,"number":11,"title":"Add PutBlockBlobIntelligently and PutBlockBlobAsList","user":{"login":"BrianMcBrayer","id":7584774,"avatar_url":"https://avatars.githubusercontent.com/u/7584774?v=3","gravatar_id":"","url":"https://api.github.com/users/BrianMcBrayer","html_url":"https://github.com/BrianMcBrayer","followers_url":"https://api.github.com/users/BrianMcBrayer/followers","following_url":"https://api.github.com/users/BrianMcBrayer/following{/other_user}","gists_url":"https://api.github.com/users/BrianMcBrayer/gists{/gist_id}","starred_url":"https://api.github.com/users/BrianMcBrayer/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/BrianMcBrayer/subscriptions","organizations_url":"https://api.github.com/users/BrianMcBrayer/orgs","repos_url":"https://api.github.com/users/BrianMcBrayer/repos","events_url":"https://api.github.com/users/BrianMcBrayer/events{/privacy}","received_events_url":"https://api.github.com/users/BrianMcBrayer/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2015-09-10T18:30:45Z","updated_at":"2015-09-14T15:55:40Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/tarwn/BasicAzureStorageSDK/pulls/11","html_url":"https://github.com/tarwn/BasicAzureStorageSDK/pull/11","diff_url":"https://github.com/tarwn/BasicAzureStorageSDK/pull/11.diff","patch_url":"https://github.com/tarwn/BasicAzureStorageSDK/pull/11.patch"},"body":"* Added derived class from BlobServiceClient\r\n* Added PutBlockBlobAsList\r\n* Added PutBlockBlobIntelligently\r\n* Unit tested / all green\r\n* Did horrible things to the code to verify it's not broken"},"comment":{"url":"https://api.github.com/repos/tarwn/BasicAzureStorageSDK/issues/comments/140123552","html_url":"https://github.com/tarwn/BasicAzureStorageSDK/pull/11#issuecomment-140123552","issue_url":"https://api.github.com/repos/tarwn/BasicAzureStorageSDK/issues/11","id":140123552,"user":{"login":"tarwn","id":1091436,"avatar_url":"https://avatars.githubusercontent.com/u/1091436?v=3","gravatar_id":"","url":"https://api.github.com/users/tarwn","html_url":"https://github.com/tarwn","followers_url":"https://api.github.com/users/tarwn/followers","following_url":"https://api.github.com/users/tarwn/following{/other_user}","gists_url":"https://api.github.com/users/tarwn/gists{/gist_id}","starred_url":"https://api.github.com/users/tarwn/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tarwn/subscriptions","organizations_url":"https://api.github.com/users/tarwn/orgs","repos_url":"https://api.github.com/users/tarwn/repos","events_url":"https://api.github.com/users/tarwn/events{/privacy}","received_events_url":"https://api.github.com/users/tarwn/received_events","type":"User","site_admin":false},"created_at":"2015-09-14T15:55:40Z","updated_at":"2015-09-14T15:55:40Z","body":"Review Notes:\r\n\r\nBlobServiceClientTestsExtensionsTests.cs\r\n\r\n* (suggestion for later) We should refactor a base test fixture or utility/harness object for the integration tests, we have the same chunk of code coped into each one\r\n* Stylistically - you have the GetProperties calls in your integration tests in the center chunk (act) when they are probably more appropriate in the last chunk (assert) since they are not part of the thing that is under test\r\n\r\n* Changes form protected set to set - I know we talked about making it easier to test. I originally made these protected set so people wouldn't modify them in the course of the normal application, but could still wrap it in a fake version of the object using inheritance. GetBlobResponse had some methods converted to virtual so they could be mocked or faked, so I would be comfortable if we continue to make these properties protected (to protect people using it from modifying values in ways they shouldn't, but expose them to be set for testing using a mock or fake which you need to do some of the time anyway)\r\n\r\n* I don't see the value in IBlobPropertiesResponse if all of the response objects are mockable/fakeable w/ inheritance - overriding the more specific concrete class allows all of the client methods to continue to return the concrete class, where using this interface would require us to make the client returns more generic (and less useful).\r\n\r\nBlobServiceClientExtensions.cs\r\n\r\n* The ternary statement in PutBlockBlobIntelligentlyAsync detracts from readability\r\n* Not sure why you have parallelized the MD5 and BlockCopy methods, this seems like a micro-optimization that is going to be inconsequential next to the performance of upload data over the wire and reading the response, at the cost of additional complexity in the code - did you have a performance test that showed a measurable performance gain?\r\n \r\nIBlobOrBlockListResponseWrapper\r\n* The client objects all use concrete mockable/fakeable objects with little or no behavior (mostly just translation of internal properties from streams or to strings so it can be constructed or presented to other formats) - the extension methods instead use interfaces for all of this. Is there value in changing the pattern to interfaces or presenting two different patterns to a potential user?\r\n\r\n"}}