QualiSystems / FluentTc

:ocean: :two_men_holding_hands: :office: Integrate with TeamCity fluently
https://www.nuget.org/packages/fluenttc
Apache License 2.0
44 stars 35 forks source link

Adding capability to retrieve build statistics #56

Closed asmoran closed 8 years ago

borismod commented 8 years ago

Hello Derrick @asmoran

First of all, welcome aboard. This makes me really happy seeing a growing community around FluentTc. Secondly, you've added a really missing functionality, your code looks clear, unit tested used and aligned with the coding standards of the project. Just a few comments from my side:

  1. The unit test GetStatistics_ByBuild_ShouldReturnZeroResults verifies that the method was called, when it's name assumes that it should verify zero results.
  2. In addition to unit tests, I tend to cover each public method in the external API (ConnectedTc) with Acceptance Test. Acceptance tests verifies that the whole functionality from the public API till the HTTP call is correct.
  3. GetStatistics returns statistics of a build, so maybe a better name would be GetBuildStatistics
  4. Last and the most important thing. One of the key principles of FluentTc is making it API easy to understand. GetStatistics returns an instance of Statistics. A user of FluentTc library might be frustrated to understand why Count is a string and why there is another property named Property. Don't you think that it would be more intuitive if the return type would be IList? Where IBuildStatistics looks like
    public interface IBuildStatistics 
    {
        string Name { get;  }
        string Value { get;  }
    }

You can see an example of such technique in GetBuilds

It was only my opinion and you may disagree with me

asmoran commented 8 years ago

Hello Boris,

I tend to agree with you on the name of GetStatistics - I was merely trying to keep the name the same as the actual API - so that users with an understanding of the API (which, let's be honest, is required to make good use of this package) can easily find the method.

This is also the reason why the Statistics object contains "Property" rather than a new "Statistic" type - aside from easy deserialization, keeping this nomenclature consistent with the underlying API makes FluentTc easier to understand.

I also agree with you on the Count issue - I was very surprised to see that you were using strings for your Count properties (see AgentWrapper, BuildWrapper, etc). I used string as it is important that this be consistent throughout the module - I will leave it to you to go through and update them to use int if that is your intent.

I've pushed up a change renaming things and adding a test in ConnectedTcTests, following the pattern you've set for the other tests in that class.