Skip to content
This repository was archived by the owner on Sep 15, 2025. It is now read-only.

Conversation

@jklausa
Copy link
Contributor

@jklausa jklausa commented Feb 19, 2019

Description

Adds support for fetching clicks and referrer data.

They're so similar in shape I took the liberty of grouping them both in one PR

Testing Details

  • Verify the code builds
  • Verify that the tests pass
  • Please check here if your pull request includes additional test coverage.

@jklausa jklausa self-assigned this Feb 19, 2019
@jklausa jklausa requested a review from a user February 19, 2019 21:03

self.title = title
self.clicksCount = clicksCount
self.clickedURL = urlString.flatMap { URL(string: $0) }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recognize why we'd use flatMap on a collection. It appears that the benefit in this case is to handle the optionality of icon & urlString, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly! It saves us doing something like:

let clickedURL: URL?
if let urlString = urlString {
    clickedURL = URL(string: urlString)
} else {
    clickedURL = nil
}

Just a bit of syntactic sugar.

public let period: StatsPeriodUnit
public let periodEndDate: Date

public let totalReferrerViews: Int
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ClicksStatsType, there appears to be an analogous property called totalClicksCount. Is this worth renaming to totalViewsCount or totalRefererredViewsCount for consistency with that and StatsReferrer (viewsCount) below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, absolutely! Thanks for catching that!

public let periodEndDate: Date

public let totalReferrerViews: Int
public let otherReferrerViews: Int
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we embrace the suggestion above, it is also applicable here.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jklausa thanks for sharing this with me. The branch builds & tests pass! The code looks good to me.

I have shared a few comments for your consideration, but none of them absolutely require a change.

@ghost
Copy link

ghost commented Feb 19, 2019

@jklausa I just noted that the hash in Podfile.lock was updated when I ran pod install to review this branch. If you find that's the case for you, would you please include a commit for that too?

@jklausa jklausa merged commit 63f022b into develop Feb 20, 2019
@jklausa jklausa deleted the feature/fetching-clicks-data branch February 20, 2019 09:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants