Pulling in story-00-matthewp on 8/6/15. #62

Merged
merged 40 commits into from Aug 7, 2015

Conversation

Projects
None yet
7 participants
@srichal
Contributor

srichal commented Aug 7, 2015



 

DrJukka and others added some commits Jul 14, 2015

@tobybrad

This comment has been minimized.

Show comment
Hide comment
@tobybrad

tobybrad Jul 30, 2015

Contributor

This form causes issues when err doesn't have .message defined (at least on iOS). Execution just stops when I come across one of these. Timeouts still work though. On a side note, assert doesn't appear to be part of the official Tape API (although it's aliased to .ok in code).

I've tended to replace these with:

  t.ok(!!err2, "Connect to bad peer should throw error");
  t.ok(err2.hasOwnProperty('message'), err2.message);

This form causes issues when err doesn't have .message defined (at least on iOS). Execution just stops when I come across one of these. Timeouts still work though. On a side note, assert doesn't appear to be part of the official Tape API (although it's aliased to .ok in code).

I've tended to replace these with:

  t.ok(!!err2, "Connect to bad peer should throw error");
  t.ok(err2.hasOwnProperty('message'), err2.message);

This comment has been minimized.

Show comment
Hide comment
@mattpodwysocki

mattpodwysocki Jul 30, 2015

Contributor

Removed all error messages instead of having constant messages with no variables

Contributor

mattpodwysocki replied Jul 30, 2015

Removed all error messages instead of having constant messages with no variables

This comment has been minimized.

Show comment
Hide comment
Member

mpodwysocki replied Jul 31, 2015

👍

mattpodwysocki and others added some commits Jul 30, 2015

Proposals for changes, please read
.gitignore - We shouldn't check in test/www/jxcore/thali since
that just repeats files that are already in the project.

build/deploytest.js - I moved this into the test directory since
it is about testing and it has paths that depend on where its
launched.

package.json - I removed this from the root of the project because
it confuses the heck out of NPM and we don't need it there since
none of our code is in the root.

mockmobile - I moved it to the test directory since it doesn't need
to be in production.

thaliscenarios.js - I don't have time now to finish this so I would
ask you to. But I introduced a structure function called
connectWithRetryTestAndDisconnect that can be used as a scafold
for all the remaining tests in the file so we don't keep on repeating
exactly the same code over and over.

thalireplicationmanager - It didn't properly handle double starts
and it wasn't cleaning itself up on stop failures.


README.md -
@mattpodwysocki

This comment has been minimized.

Show comment
Hide comment
@mattpodwysocki

mattpodwysocki Jul 31, 2015

Contributor

Strings are not Errors, please do new Error('There is already an existing serverBridge instance'). Also nit, use ' instead of "

Strings are not Errors, please do new Error('There is already an existing serverBridge instance'). Also nit, use ' instead of "

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg Jul 31, 2015

Member

Why prefer ' to "?

Member

yaronyg replied Jul 31, 2015

Why prefer ' to "?

This comment has been minimized.

Show comment
Hide comment
@mattpodwysocki

mattpodwysocki Jul 31, 2015

Contributor

Convention that I always follow from the Google JavaScript coding standards http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Strings

Contributor

mattpodwysocki replied Jul 31, 2015

Convention that I always follow from the Google JavaScript coding standards http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Strings

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg Jul 31, 2015

Member

Seem reasonable enough. Please consider this PR as review comments so take or leave as necessary. Right now I'm trying to pick up install off the floor. :)

Member

yaronyg replied Jul 31, 2015

Seem reasonable enough. Please consider this PR as review comments so take or leave as necessary. Right now I'm trying to pick up install off the floor. :)

This comment has been minimized.

Show comment
Hide comment
@mattpodwysocki

mattpodwysocki Jul 31, 2015

Contributor

👍

Contributor

mattpodwysocki replied Jul 31, 2015

👍

This comment has been minimized.

Show comment
Hide comment
Member

mpodwysocki replied Jul 31, 2015

👍

@srichal srichal added the in progress label Aug 7, 2015

@srichal srichal merged commit e7e8ed6 into story_00_srichal Aug 7, 2015

@srichal srichal removed the in progress label Aug 7, 2015

@mpodwysocki mpodwysocki deleted the story_00_matthewp branch Dec 10, 2015

vasilevskayaem added a commit that referenced this pull request Nov 17, 2016

vasilevskayaem added a commit that referenced this pull request Nov 17, 2016

vasilevskayaem added a commit that referenced this pull request Nov 24, 2016

vasilevskayaem added a commit that referenced this pull request Nov 24, 2016

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