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 _d param to indicate manual passing of a timestamp #12

Merged
merged 3 commits into from
Nov 20, 2014

Conversation

krizon
Copy link
Contributor

@krizon krizon commented Oct 20, 2014

Added the _d parameter for indicating the manual passing of a timestamp as described in the API Specifications.

@krizon
Copy link
Contributor Author

krizon commented Nov 18, 2014

@bhardin sorry to ping you but could you review this PR?

@bhardin
Copy link
Contributor

bhardin commented Nov 18, 2014

Im away on vacation. @percyhanna can you review and merge?


Brett Hardin
http://bretthard.in

On Mon, Nov 17, 2014 at 11:55 PM, Kristian Zondervan
notifications@github.com wrote:

@bhardin sorry to ping you but could you review this PR?

Reply to this email directly or view it on GitHub:
#12 (comment)

$isManualTime = false;
} else {
$isManualTime = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The indentation on this block is inconsistent.

@percyhanna
Copy link
Contributor

I think we should add some more tests to some of this, but otherwise it looks great. At a minimum we should add tests for null time values (right now the tests are passing in 0). I would also like to see more tests in KISSmetrics\Transport\Sockets, but I don't think that needs to be part of this PR (unless you want to add some). I'm thinking specific tests for the new behaviour of _d, as well as other tests for different end points of our tracking service.

@krizon
Copy link
Contributor Author

krizon commented Nov 20, 2014

I updated the PR. Fixed the indentation and provided extra Client tests covering the null value for time. I agree the transports need more tests but is out of scope for now. Thanks.

@percyhanna
Copy link
Contributor

Awesome, thanks for the updates Kristian. This looks great.

percyhanna added a commit that referenced this pull request Nov 20, 2014
Add _d param to indicate manual passing of a timestamp
@percyhanna percyhanna merged commit 0d15c78 into kissmetrics:master Nov 20, 2014
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.

None yet

3 participants