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

Add BrowserFetcher bindings #42

Merged
merged 12 commits into from Mar 26, 2018

Conversation

Projects
None yet
2 participants
@jihchi
Copy link
Collaborator

jihchi commented Mar 19, 2018

Fix #40

--

These bindings increase our unit test duration.
screenshot_2018-03-26_22-41-41_ps9gx

@zploskey

This comment has been minimized.

Copy link
Owner

zploskey commented Mar 22, 2018

These look good to me. It's debatable whether we should bother typing the platform return value beyond a string. I doubt this will be used often. Would be fine with it as is.

Archie Lee

@jihchi jihchi force-pushed the jihchi:40_bindings_for_browserfetcher branch from dadad18 to d69fa84 Mar 25, 2018

| [@bs.as "mac"] `Mac
| [@bs.as "linux"] `Linux
| [@bs.as "win32"] `Win32
| [@bs.as "Win64"] `Win64

This comment has been minimized.

@zploskey

zploskey Mar 25, 2018

Owner

win64 should be all lower case. If you just make the polymorphic variants all lower case and the same as the string doesn't it convert them to the right string? No need to use bs.as.

This comment has been minimized.

@jihchi

jihchi Mar 26, 2018

Author Collaborator

I thought variant constructor need to be capitalized. tried lower case but without success.

This comment has been minimized.

@zploskey

zploskey Mar 26, 2018

Owner

Polymorphic variants don't need to be capitalized. For example: https://github.com/bs-puppeteer/bs-puppeteer/blob/master/src/ConsoleMessage.re

This comment has been minimized.

@jihchi

jihchi Mar 26, 2018

Author Collaborator

Oh! That works, Thanks!

@jihchi

This comment has been minimized.

Copy link
Collaborator Author

jihchi commented Mar 26, 2018

Hi @zploskey ,

PR is done.

};

[@bs.obj]
external makeBrowserFetcherOption :

This comment has been minimized.

@zploskey

zploskey Mar 26, 2018

Owner

To be consistent this should be makeBrowserFetcherOptions, plural.

This comment has been minimized.

@jihchi

jihchi Mar 26, 2018

Author Collaborator

Fixed in commit 784ba5c

@zploskey zploskey merged commit cdd11a4 into zploskey:master Mar 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jihchi jihchi deleted the jihchi:40_bindings_for_browserfetcher branch Mar 29, 2018

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