Schniz / bs-faker

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

Remove jsnullablet #43

Closed idkjs closed 4 years ago

idkjs commented 4 years ago

Per https://github.com/Schniz/bs-faker/pull/42#issuecomment-565809527, changed getOrRaise to Belt.Option.getExn. Removes 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.

Schniz commented 4 years ago

And I think now, with the recent changes, the changes in the tests can be reverted to make them pass again

idkjs commented 4 years ago

I think we are good now.

Schniz commented 4 years ago

Cool! does everything still works if we do git checkout origin/master -- src/*.rei? This PR still have changes in the interface files and I think that keeping them unchanged due to formatting (or maybe just removing type nullable = Js.Nullable.t) will have a better "git hygiene"

I'd be also happy to get the code reformatted with bsrefmt --in-place src/*.re*

idkjs commented 4 years ago

@Schniz What doe git checkout origin/master -- src/*.rei do ?

Schniz commented 4 years ago

revert all changes in interface files

idkjs commented 4 years ago

I reverted but then removed the [bs.deriving abstract] and changed the [bs.optional's to option(string) in Fakers.re type card_address since the point here was to remove those calls. Is this what you expected? I am assuming the point was to reformat the .rei files. Is this correct?

Schniz commented 4 years ago

The reason I wanted the *.rei to be almost unchanged is because it means that we can bump a minor version and not a major version, because there is no change in the public API 😄