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

set location should not use comma #532

Merged
merged 4 commits into from
Feb 13, 2018
Merged

Conversation

hiaw
Copy link
Contributor

@hiaw hiaw commented Jan 22, 2018

using comma instead of dot will cause fbsimctl to fail.

using comma instead of dot will cause fbsimctl to fail.
@rotemmiz
Copy link
Member

CI should be green now, can you please rebase the PR and rerun the tests ?

@LeoNatan
Copy link
Contributor

Can you please explain what is going on here? What the issue is and what the fix is? I don't understand.

@hiaw
Copy link
Contributor Author

hiaw commented Jan 29, 2018

The current code will give error if giving geo coordinate with decimal points. The PR should fix it.

screen shot 2018-01-29 at 1 16 07 pm

screen shot 2018-01-29 at 1 16 30 pm

@hiaw
Copy link
Contributor Author

hiaw commented Jan 29, 2018

@rotemmiz this PR is rebased to the master branch.

But CI will fail because the test project's detox is based on the npm version instead of building from this code change.

https://github.com/wix/detox/blob/master/detox/test/package.json#L19

@rotemmiz
Copy link
Member

Why do you suspect it uses the npm version?
We use lerna to control the project dependencies. Projects in the monorepo which are declared in lerna.json are being linked inside node_modules rather than being downloaded from npm.

@hiaw
Copy link
Contributor Author

hiaw commented Jan 29, 2018

ok. i'm not correct.

The CI failed because of wrong username & password while trying to publish the doc?
https://travis-ci.org/wix/detox/jobs/334522771

@rotemmiz
Copy link
Member

@DanielMSchmidt can you please help with this?

@DanielMSchmidt
Copy link
Contributor

@rotemmiz two fixes, one is in #544 so it doesn't even run for the PR.
The second one is we (someone with full access to travis 😉 ) need to set $GIT_USER and $GIT_TOKEN with valid credentials that allow a push to the repo (for the github-pages branch). For more details, please see the documentation of Docusaurus

@rotemmiz
Copy link
Member

Hi again,
Please rebase your branch so we can make sure tests pass.

@hiaw
Copy link
Contributor Author

hiaw commented Feb 12, 2018

ok. done. no conflict & all test passed

@LeoNatan LeoNatan merged commit 9208774 into wix:master Feb 13, 2018
@LeoNatan
Copy link
Contributor

Thanks, merged

@wix wix locked and limited conversation to collaborators Jul 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants