Adding async support and tests for running experiments #10
Conversation
| @@ -0,0 +1,6 @@ | |||
| language: java | |||
| jdk: | |||
There was a problem hiding this comment.
Thanks for adding Travis support, I'll enable this repo for it. Can you put this change up as a separate PR?
There was a problem hiding this comment.
I am not sure that my brain can handle doing another squash and commit split.
There was a problem hiding this comment.
I think I should be able to add it without causing any issues on this branch. In the worst case it'll probably just require you to rebase. I'd like to have Travis enabled so that it can run the tests on this branch without me having to pull it down.
There was a problem hiding this comment.
you should just be able to do:
git fetch upstream/master
git rebase upstream/master
git push -f
and the branch conflicts from the travis changes should go away.
and then do
There was a problem hiding this comment.
You might want to check the project settings to see if you allow the new git squash PR feature. https://github.com/blog/2141-squash-your-commits Haven't tried this on a fork, so it might only work on local branches
There was a problem hiding this comment.
the conflict it resolved but I didn't squash your review changes, hoping that is okay and we can just proceed and carry along the extra commit history which shouldn't cause any issues.
|
Thanks for this PR, won't be able to review it in full for a couple of days though |
|
That is fine. The library isn't very useful atm as we have some time critical tests we want to execute and doubling execution time of control vs campaign would probably make the business angry. :) |
| candidateTimer = metrics.timer(MetricRegistry.name(NAMESPACE_PREFIX, this.name, "candidate")); | ||
| mismatchCount = metrics.counter(MetricRegistry.name(NAMESPACE_PREFIX, this.name, "mismatch")); | ||
| totalCount = metrics.counter(MetricRegistry.name(NAMESPACE_PREFIX, this.name, "total")); | ||
| this.name = name; |
There was a problem hiding this comment.
what's with all the spacing changes? Could you change your IDE to have two spaces for tabs instead of four? IIRC this should have two spaces for tab, not four as I think this tries to introduce
There was a problem hiding this comment.
I am pretty sure 4 spaces (preferred) or Tab is the standard. I haven't been anywhere that uses 2 spaces for java code. I will update if that makes your eyeballs happier. (Although I am pretty sure if would take you less time to fix after merging than me going through updating and rebasing, squashing etc)
There was a problem hiding this comment.
It appears that you were using 4 space indent for method code then +2 for method body. This seems inconsistent and it seems like consistently using 4 would be preferred for all code to keep IDEs like IntelliJ easy to use when editing.
There was a problem hiding this comment.
I tired all combinations of tab/indent of 4/2 and can't get it to do what you are asking.
There was a problem hiding this comment.
fair enough we can leave the spacing as you have it
|
overall this looks great, thanks again for this change. I left a few minor comments + please fix the spacing issues and this should be good to merge. |
| @@ -4,7 +4,7 @@ | |||
|
|
|||
| <groupId>com.github.rawls238</groupId> | |||
| <artifactId>Scientist4J</artifactId> | |||
| <version>0.1</version> | |||
| <version>0.2</version> | |||
There was a problem hiding this comment.
also change the version in the README?
…he readme Typo in readme Adding travis build Triggering build Maven builds are silly (gradle ftw) verify attempts to sign, use test for travis Moving to profile based maven to stop GPG failing build Bumping version to 0.2 Tell travis to not use java 7 Why is maven so fickle Reverting gpg dep in maven fine and removing profiles now that travis overrides the install Typo in readme Moving to profile based maven to stop GPG failing build Bumping version to 0.2 Tell travis to not use java 7 Why is maven so fickle Reverting gpg dep in maven fine and removing profiles now that travis overrides the install
… overrides the install
|
@ctoestreich the new git PR squash feature should be enabled for this repo. This looks ready to merge to me, is there anything else you want to change? |
|
Have at it. We can change back the random async stuff later if we add more features. |
|
@ctoestreich I just published the new version to maven central |
|
@ctoestreich is this working as expected for you guys? |
Squashed commit from previous PR