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.

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

Add repository traffic preview #1457

Merged
merged 11 commits into from
Sep 28, 2016

Conversation

martinscholz83
Copy link
Contributor

fix #1447

Implement client for the new repository traffic api.

@martinscholz83
Copy link
Contributor Author

Maybe someone has a better naming for TrafficeDayOrWeek 😸

@martinscholz83 martinscholz83 force-pushed the repository-traffic-preview branch from 024ce12 to 23f22e2 Compare September 2, 2016 12:23
@martinscholz83 martinscholz83 changed the title Add repository traffic preview [WIP] Add repository traffic preview Sep 2, 2016
@martinscholz83 martinscholz83 force-pushed the repository-traffic-preview branch from bce79eb to c9a1302 Compare September 5, 2016 07:55
@martinscholz83 martinscholz83 force-pushed the repository-traffic-preview branch from c9a1302 to 61f6a67 Compare September 5, 2016 07:58
@martinscholz83
Copy link
Contributor Author

martinscholz83 commented Sep 5, 2016

@ryangribble, what about integration tests for this feature. The only thing we can test is that the result is not null 😑. And of course the result count is 0.. Should i do that?

@ryangribble
Copy link
Contributor

I guess integration tests still prove everything is wired up correctly, even if there aren't any meaningful stats...

I take it we can't point the calls at an existing repository due to permissions?

@martinscholz83
Copy link
Contributor Author

Yeap, you need to have push access.

@martinscholz83
Copy link
Contributor Author

image

@martinscholz83
Copy link
Contributor Author

Hi @ryangribble, is this ready for 🚢?

@ryangribble
Copy link
Contributor

Hi @maddin2016

Do you think you could add some integration tests then flag them as Skipped due to requiring admin permissions. This way I can at least run these locally (after changing the repostory to something I do have admin permissions on and unskipping the tests) to ensure the functionality is working as intended?

@ryangribble
Copy link
Contributor

ryangribble commented Sep 24, 2016

Hey @maddin2016

Thanks for adding the tests - lucky we did because there was actually an issue with timestamps! 😀

I pulled everything down and ran through it, and these are the things I changed:

  • RepositoryId was recently changed from int to long in Change repository id from int to long #1445 so the repositoryId parameters in this new API need to be long now too
  • The timestamp field in the Api responses is in unix utc time, and doesn't deserialize into a DateTimeOffset directly, causing exceptions on the View and Clone methods when the repository actually had some data to return for these calls
  • I wasnt a fan of naming the item level classes View and Clone since they are a bit too generic IMO
  • I also thought naming some methods GetAllxxx() when they don't support pagination we can use the new [ExcludeFromPaginationConventionTests] so we can call them GetReferrers() and GetPaths() which is more inline with what they do (it's not all referrers, just referrers from last 14 days etc)
  • Integration tests were added for owner/name methods but not for repositoryId methods

As I was working through things I ended up making local fixes anyhow, so I've pushed a commit that addresses these already if you want to cherry pick it in: TattsGroup@b145bb3

If you are interested in how we handle the timestamp thing, basically in other areas of the codebase we have a pattern already, where we add a long TimestampAsUtcEpochSeconds field to take the deserialized value (using the Parameter override to map it to timestamp) and then add a DateTimeOffset Timestamp getter property that uses a helper function to convert the unix time into DateTimeOffset. it looks something like this (and you'll see these fixes in my commit):

        [Parameter(Key = "ignoreThisField")]
        public DateTimeOffset Timestamp
        {
            get { return TimestampAsUtcEpochSeconds.FromUnixTime(); }
        }

        [Parameter(Key = "timestamp")]
        public long TimestampAsUtcEpochSeconds { get; protected set; }

Let me know what you think of my changes

@martinscholz83
Copy link
Contributor Author

Hi @ryangribble, thanks for your review. I have read on the format of timestamp. Is there any explanation why it's this format? And yes, View is realy a bit to common 😁. The renaiming of GetAllxxx() makes sense. I already have this in mind but i don't know wheter it woul be correct. So many thanks for your fixes 👍

Remove GetAll naming of endpoints and add to PaginationTest exclusions
Rename View and Clone classes to be more specific
Add handling of TimeStamp fields being UtcUnix time
Add integration tests for repositoryId methods
@martinscholz83
Copy link
Contributor Author

And the renaming of the repsitory id's i have forgotten, because i already did this in another PR 😅

@ryangribble ryangribble merged commit a57fb12 into octokit:master Sep 28, 2016
@ryangribble
Copy link
Contributor

ryangribble commented Sep 28, 2016

Thanks for another great contribution @maddin2016 😀

LGTM

@ryangribble ryangribble changed the title [WIP] Add repository traffic preview Add repository traffic preview Sep 29, 2016
@martinscholz83
Copy link
Contributor Author

Many thanks @ryangribble
highfive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Repository Traffic preview API
2 participants