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

Topkg #337

Closed
wants to merge 5 commits into from
Closed

Topkg #337

wants to merge 5 commits into from

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Oct 24, 2016

this got slightly bigger than expected, I also removed the pack (and provided a main interface, tls.mli). compilation with principal fails (thus not included in _tags), and there are various warnings which I'll fix subsequently

@hannesm
Copy link
Member Author

hannesm commented Oct 24, 2016

@pqwy if you feel like, pls review+merge

@pqwy
Copy link
Contributor

pqwy commented Oct 25, 2016

Why is this so ginormous?

@hannesm
Copy link
Member Author

hannesm commented Oct 25, 2016

I removed pack, put a tls.mli together by copy&pasting, and prefixed the modules with Tls_.

@pqwy
Copy link
Contributor

pqwy commented Oct 25, 2016

Yes. But why?

@hannesm
Copy link
Member Author

hannesm commented Oct 25, 2016

  • remove pack: since there's no reason for pack (and I had the impression it is in general a good property to not pack libraries) -- unlikely that OCaml will be able to remove modules during link time (since Engine basically needs all the modules)
  • rename, prefix with Tls_: since in current master if you try make doc, it will fail due to the module Config being defined in Tls and elsewhere. pack seems to work fine in some scenarios to hide the internal name, but not always (and OCaml lacks namespacing). again, I thought this is best practises
  • Tls.mli to provide an API, a single file where potential clients of this package can dig into

@pqwy
Copy link
Contributor

pqwy commented Oct 25, 2016

pack is a pretty neutral thing, orthogonal to other concerns.

Except if you remove it, hide other modules, and slap a common mli on top, it's a game changer. Previously, it used to be the case that "we didn't have a clear separation between the public and private API" -- as both of us repeatedly wrote -- because the parts saw each other through the same interfaces that the clients saw them through. Since other modules now don't go through tls.mli, it is precisely this: canonical public interface.

So this PR piggy-backs an extremely important thing, introducing an explicit public API where there was none, onto a set of changes ostensibly about the build system switch. For reasons that are beyond me.

And it does so in the worst possible way. Look at the new, explicitly public, interface. It's leaking the internals left and right. Of course it is, it's a concatenation of private APIs.

It made me chuckle a bit. I seem to remember a published paper about how the moral equivalent of that file is in fact short and simple.

So could you please cut down the PR to deal with changing the actual build system, while producing the same artifact. And then we can talk about what's the final public footprint of the entire library.

@hannesm
Copy link
Member Author

hannesm commented Oct 26, 2016

I agree that the public API needs more thought (IMHO we can do this incrementally on master, but you seem to disagree). I started in #338 with fixes for travis (and moving the examples to mirage-dev).

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

Successfully merging this pull request may close these issues.

None yet

2 participants