/speed

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

code review: add case int; TypeSwitchGuard short variable declaration #14

Closed
wants to merge 2 commits into
from

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@peterGo @coveralls @suyash
@peterGo
Contributor

peterGo commented Jul 18, 2016

No description provided.

peterGo added some commits Jul 18, 2016

@peterGo
Merge pull request #1 from performancecopilot/master 
update fork
88ec8c2
@peterGo
code review: add case int; TypeSwitchGuard short variable declaration

Some checks were not successful

1 failing and 1 successful checks
coverage/coveralls — Coverage decreased (-0.5%) to 64.836%
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
f14ec12
@coveralls

coveralls commented Jul 18, 2016 • edited

Coverage Status

Coverage decreased (-0.5%) to 64.836% when pulling f14ec12 on peterGo:metrics-WriteVal into 15eb629 on performancecopilot:master.
@suyash
Collaborator

suyash commented Jul 18, 2016

A couple of things

  1. We will never write int, the resolveInt function takes care of resolving int to one of int32, int64, uint32 and uint64, and that is kind of what we are going for, it removes any ambiguity related to the type, if you specified the MetricType as Uint32Type, the only value you can get is uint32, not uint or int. The rest is upto the user.
  2. Please always submit pull requests after rebasing with the latest upstream. The commit Merge pull request #1 from performancecopilot/master looks odd in the git history. Just add an upstream remote as the https address to this repository, pull to master and rebase your branch to it before submitting the PR.

But thanks for the general idea, looks cool 👍

@suyash suyash closed this Aug 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment