Schniz / bs-faker

Faker.js bindings for BuckleScript in Reason
MIT License
17 stars 8 forks source link

Updated bindings to bs-platform@7.0.1 #42

Closed idkjs closed 4 years ago

idkjs commented 4 years ago

I was trying use this in a project running bs-platform@7.0.1 and ran up against some errors. I updated the package so that all the tests pass. There is one warning on using a deprecated Js.Re.test as seen below. I haven't changed it since the test runs and I am not a test running genius.

I read you contributor guide. Its says to note how many binding you contributed. I have updated the bindings that were already there so there is nothing new. Please indicate if that request is relevant.

➜  bs-faker-bsp@701 [master*]yarn test
yarn run v1.19.1
warning ../../package.json: No license field
$ bsb -make-world && jest
[5/5] Building src/jest.cmj
[135/135] Building __tests__/Internet_test-BsFaker.cmj

  Warning number 3
  /Users/prisc_000/Downloads/bs-faker-bsp@701/__tests__/Internet_test.re 104:49-58

  102 ┆ test("accept pattern", () => {
  103 ┆   let pattern = [%re "/[a-b0-1]+/"];
  104 ┆   expect(Internet.password(~pattern, ()) |. Js.Re.test(pattern)) ===
        true;
  105 ┆ });
  106 ┆ test("accept prefix", () =>

  deprecated: Js.Re.test
Please use Js.Re.test_ instead

 PASS  lib/js/__tests__/Finance_test.bs.js
 PASS  lib/js/__tests__/Date_test.bs.js
 PASS  lib/js/__tests__/Internet_test.bs.js
 PASS  lib/js/__tests__/Helpers_test.bs.js
 PASS  lib/js/__tests__/Lorem_test.bs.js
 PASS  lib/js/__tests__/Address_test.bs.js
 PASS  lib/js/__tests__/Random_test.bs.js
 PASS  lib/js/__tests__/Image_test.bs.js
 PASS  lib/js/__tests__/System_test.bs.js
 PASS  lib/js/__tests__/Commerce_test.bs.js
 PASS  lib/js/__tests__/Company_test.bs.js
 PASS  lib/js/__tests__/Name_test.bs.js
 PASS  lib/js/__tests__/Phone_test.bs.js
 PASS  lib/js/__tests__/Hacker_test.bs.js
 PASS  lib/js/__tests__/Database_test.bs.js
 PASS  lib/js/__tests__/Faker_test.bs.js
 PASS  lib/js/__tests__/Locale_test.bs.js

Test Suites: 17 passed, 17 total
Tests:       211 passed, 211 total
Snapshots:   0 total
Time:        10.23s
Ran all test suites.
✨  Done in 14.26s.
sync commented 4 years ago

Either this or keep bs-abstract, there isn’t a good solution in between

idkjs commented 4 years ago

bs-absract isnt used in this project. Do you mean bs-platform? Not following?

sync commented 4 years ago

Sorry I meant [@bs.deriving abstract]

idkjs commented 4 years ago

So we don't trust the tests then?

sync commented 4 years ago
test(".createCard", () =>
    expect(Js.typeof(Helpers.createCard())) === "object"
  );

Doesn’t test accessor usage like .streetD i think 🤔

idkjs commented 4 years ago

CreateCard test passes:

Test:

open Jest;
open Expect;
open! Expect.Operators;

describe("BsFaker.createCard", () => {
  test(".createCard", () =>
    expect(Js.typeof(Helpers.createCard())) === "object"
  );
});
[88/88] Building __tests__/CreateCard_test-BsFaker.cmj
 PASS  lib/js/__tests__/Date_test.bs.js (5.956s)
 PASS  lib/js/__tests__/Finance_test.bs.js (5.987s)
 PASS  lib/js/__tests__/Internet_test.bs.js (6.015s)
 PASS  lib/js/__tests__/Helpers_test.bs.js
 PASS  lib/js/__tests__/Lorem_test.bs.js
 PASS  lib/js/__tests__/Address_test.bs.js
 PASS  lib/js/__tests__/Random_test.bs.js
 PASS  lib/js/__tests__/Image_test.bs.js
 PASS  lib/js/__tests__/System_test.bs.js
 PASS  lib/js/__tests__/Commerce_test.bs.js
 PASS  lib/js/__tests__/Name_test.bs.js
 PASS  lib/js/__tests__/Company_test.bs.js
 PASS  lib/js/__tests__/Phone_test.bs.js
 PASS  lib/js/__tests__/Database_test.bs.js
 PASS  lib/js/__tests__/Hacker_test.bs.js
 PASS  lib/js/__tests__/Locale_test.bs.js
 PASS  lib/js/__tests__/Faker_test.bs.js
 PASS  lib/js/__tests__/CreateCard_test.bs.js

Test Suites: 18 passed, 18 total
Tests:       212 passed, 212 total
Snapshots:   0 total
Time:        14.537s
Ran all test suites.
✨  Done in 18.39s.
Schniz commented 4 years ago

Doesn’t test accessor usage like .streetD i think 🤔

I'm not quite sure I understand what you meant, but it sounds like a valuable review. Can you tell us more about what you mean?

sync commented 4 years ago

I am sorry I completely forgot about I’ll have a look at it today and confirm

sync commented 4 years ago

Please go ahead and merge this anything marked optional here is fine with new bs platform object feature. Amazing work converting this to latest bsb !!

idkjs commented 4 years ago

Please go ahead and merge this anything marked optional here is fine with new bs platform object feature. Amazing work converting this to latest bsb !!

@sync @Schniz your comment above got me thinking about whether we could remove some of the Js.Nullable.t calls in bs-platform@7.0.1. I went and checked and it works. Obviously, this is bs.platform@7.01 nit picking. This will make the previous merge fully idiomatic bs.platform@7.01. Tests are passing after I added a getOrRaise function in Helper.re Should I make a pull request? https://github.com/idkjs/bs-faker/tree/remove-jsnullablet

Schniz commented 4 years ago

Yeah sure! a PR would be wonderful 😄 I think the Helper.getOrRaise can be Belt.Option.getExn