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

react-native / port: convert string to integer #87

Merged
merged 3 commits into from
Aug 5, 2019
Merged

react-native / port: convert string to integer #87

merged 3 commits into from
Aug 5, 2019

Conversation

kuhnchris
Copy link
Contributor

Reffering to #86

Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

Travis is failing due to commitlint, I removed the commitlint in master as it's not necessary, if you want to rebase the tests will no longer be blocked by the commitlint check.

I ran the tests locally and there are a few failing due to the change from a string to a number, so those will need to be updated. Additionally, can you ensure the jsdocs are updated to represent the port as a Number instead of String? The docs are generated from those.

@kuhnchris
Copy link
Contributor Author

Sure, can take care of that.

@kuhnchris
Copy link
Contributor Author

fixed jsdoc and fixed tests.
I did however not finish running tests, it keeps failing for me:

  109 passing (278ms)
  6 pending

Test Browser


i 「wdm」:
i 「wdm」: Compiled successfully.
i 「wdm」: Compiling...
i 「wdm」:
i 「wdm」: Compiled successfully.

  0 passing (1m)

Command failed: karma start C:\Users\kuhnc\Documents\js-multiaddr\node_modules\aegir\src\config\karma.conf.js --files-custom --log-level error
npm ERR! Test failed.  See above for more details.

(maybe because this is a windows machine?)

Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on this! This looks good.

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.

2 participants