Add Support for the Github Traffic APIs #49
Conversation
| ) | ||
|
|
||
| $params = @{ | ||
| 'UriFragment' = "repos/$OwnerName/$RepositoryName/traffic/views`?" + ($getParams -join '&') |
There was a problem hiding this comment.
Seems like $getParams is used elsewhere with multiple elements and we're just matching a style here. It seems unnecessary and I would support just adding "per=$Per" to the rest of the UriFragment.
There was a problem hiding this comment.
Totally reasonable feedback. I'm preparing to publish the 0.4.0 update. I can make that minor change.
| Tests for GitHubRepositoryForks.ps1 module | ||
| #> | ||
|
|
||
| [String] $root = Split-Path -Parent (Split-Path -Parent $Script:MyInvocation.MyCommand.Path) |
There was a problem hiding this comment.
Generally, don't enforce a strict type for variables. PowerShell is designed so that the type of a variable can change. If you want to make it clear what the type is of an expression, that's usually on the right side, ie $root = [string] <expr>
| Tests for GitHubRepositoryForks.ps1 module | ||
| #> | ||
|
|
||
| [String] $root = Split-Path -Parent (Split-Path -Parent $Script:MyInvocation.MyCommand.Path) |
There was a problem hiding this comment.
This works, but I feel like it's just clearer to say Join-Path "..\..\" $MyInvocation.MyCommand.Path. The result is the same and it's easier to see that we want to jump up two directories.
| Tests for GitHubRepositoryForks.ps1 module | ||
| #> | ||
|
|
||
| [String] $root = Split-Path -Parent (Split-Path -Parent $Script:MyInvocation.MyCommand.Path) |
There was a problem hiding this comment.
Look into $PSScriptRoot, because it seems like it will simplify some of this.
| Tests for GitHubRepositoryForks.ps1 module | ||
| #> | ||
|
|
||
| [String] $root = Split-Path -Parent (Split-Path -Parent $Script:MyInvocation.MyCommand.Path) |
There was a problem hiding this comment.
This variable is already script-scope so there's no need to make it explicit.
| function Initialize-AppVeyor | ||
| { | ||
| <# | ||
| .SYNOPSIS |
There was a problem hiding this comment.
We should get out of the habit of having a synopsis that's almost entirely duplicative of the description. If it doesn't add value, let's not do it.
| Context 'When initially created, there are no referrers' { | ||
| $referrerList = Get-GitHubReferrerTraffic -Uri $repo.svn_url | ||
|
|
||
| It 'Should return expected number of referrers' { |
There was a problem hiding this comment.
nit: It feels like Context and It aren't being treated quite right for these tests. The context shouldn't be When initially created, there are no referrers, because that's very specific. How would you say this if you were having a natural conversation? The context is just that the repository has been initially created and, within that context, getting referrer traffic should return zero referrers. It doesn't seem right to tie 'there are no referrers' as part of the context, because we can test for other unrelated things within that context. That is:
Describe 'getting the referrer list' {
Context 'for a new repository' {
It 'should have 0 referrers' …
It 'should throw a welcome party' ...
}
}| Describe 'Getting the popular content over the last 14 days' { | ||
| $repo = New-GitHubRepository -RepositoryName ([Guid]::NewGuid().Guid) -AutoInit | ||
|
|
||
| Context 'When initially created, there are is no popular content' { |
| Context 'When initially created, there are no referrers' { | ||
| $referrerList = Get-GitHubReferrerTraffic -Uri $repo.svn_url | ||
|
|
||
| It 'Should return expected number of referrers' { |
There was a problem hiding this comment.
It is too generic here to be useful. keep in mind that the strings used here are what will appear on the console. For this test, it's specific that there should be zero referrers, so the It description should be clear also.
| $referrerList = Get-GitHubReferrerTraffic -Uri $repo.svn_url | ||
|
|
||
| It 'Should return expected number of referrers' { | ||
| @($referrerList).Count | Should be 0 |
There was a problem hiding this comment.
In general, you want to force the array type as early as possible. In the call:
$referrerList = Get-GitHubReferrerTraffic -Uri $repo.svn_urlThe type of $referrerList is now ambiguous and this is solved by forcing the array conversion when wanting to test the result, that is @($referrerList).Count | Should be 0. The good practice to get into, is to make the type of $referrerList be clear from the start, that is:
$referrerList = @(Get-GitHubReferrerTraffic -Uri $repo.svn_url)Now it's exactly clear that $referrerList is an array, and there's no worry about directly saying:
$referrerList.Count | Should be 0Another reason to do this, is that the name $referrerList implies a collection, so we should be explicit about the type. It's a best-practice to carry across all your powershell changes.
| Context 'When initially created, there are no views' { | ||
| $viewList = Get-GitHubViewTraffic -Uri $repo.svn_url | ||
|
|
||
| It 'Should return 0 in the count property' { |
There was a problem hiding this comment.
This doesn't make sense because $viewList isn't even guaranteed to be an array. 'should return 0 views` seems more appropriate.
Added support for all of the Github Traffic APIs.
Also added GitHubTraffic.tests.ps1 for testing. These tests will only ensure that we can still talk to the Github APIs since there is not a reliable way to test the data that the end point would return on a new or existing repository.