Implement interfaces (issue #40)

[seprich]

No description provided.

[coveralls]

Pull Request Test Coverage Report for Build 75

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 98.71%

---

Totals
Change from base Build 70:	0.0%
Covered Lines:	136
Relevant Lines:	137

---

💛 - Coveralls

[gilbert]

Just to confirm, these are generated? (It's been a while since I used reason)

[seprich]

I used editor plugin type hints (which uses bsb), and wrote manually according to those, however I did change the signature of make from (('a => unit) => 'b) => t('a) into (('a => unit) => unit) => t('a) which makes more sense because the value 'a is the only one which is used or useful. However such a change could break someones code (and required editing on the testing code in this PR) and therefore is not backwards compatible. Another similar, but which I didn't change, is with tapOk and tapError , the function given as second parameter is 'a =>'b so it may return anything, which is not however used for anything. In such case 'a => unit would be IMO more sensible, but again that change could break someones code.

Maybe I should change the signature of make back to the backwards compatible (('a => unit) => 'b) => t('a) form and make a separate PR about sanitizing signatures which could be then evaluated separately?

[scull7]

Does this modify the interface?

[seprich]

yes it does. Ok I'll change this PR to ensure backwards compatibility

[seprich]

The interface is now carefully implemented to be identical with type inference done by bsb, so this PR should be 100% backwards compatible at this point. Assuming though that the implementation details of type t('a); are not intended to be exposed.. I hope that is a sensible assumption.

[scull7]

[briangorman]

The interface is now carefully implemented to be identical with type inference done by bsb, so this PR should be 100% backwards compatible at this point. Assuming though that the implementation details of type t('a); are not intended to be exposed.. I hope that is a sensible assumption.

Did you you create this by using the automatic interface generator?

https://bucklescript.github.io/docs/en/automatic-interface-generation

[scull7]

@briangorman why is the automatic creation of the interface file a concern? Standardization?

[briangorman]

@briangorman why is the automatic creation of the interface file a concern? Standardization?

Just making sure that the interface isn't changing

[scull7]

It would be an extraordinary case where the interface could change. And in the case where it would change it would be that the specified interface is more specific than the inference done by bsb. If there's concern over interface incompatibility we should bump the major version number. However, I don't see anything which is different.

[scull7]

@briangorman Can we go ahead and merge this?

[briangorman]

@scull7 I haven't had time to check this, but if you have taken a look and the interface didn't change I have no problem merging this 👍

[scull7]

As far as I can tell, and the tests can tell, the interface has not changed.

[briangorman]

The PR looks good. Only thing that seems to differ from the build in generator is the parameter order

e.g.

let map: (t('a), 'a => 'b) => t('b);

versus (generated)

let map: (t('b), 'b => 'a) => t('a); 

[briangorman]

Thanks for the PR,

Could you please generate this file by running the bsb inference? The parameter names to functions are ordered differently than those generated by bsb and I think we should try to stay as consistent with community conventions as possible.

e.g.

let map: (t('a), 'a => 'b) => t('b);

versus (generated)

let map: (t('b), 'b => 'a) => t('a); 

Running the inference is very quick.