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

Remove camlp4 reference from the manual #457

Merged
merged 4 commits into from Jul 23, 2017

Conversation

bobbypriam
Copy link
Collaborator

Solves #449.

Removes camlp4 reference, since its use has been deprecated. The "Correspondence table" is also updated to use the Ppx syntax.

@bobbypriam
Copy link
Collaborator Author

There is still one reference left of camlp4 in The logging facility section. I'm not sure what to do with it, should I just delete it altogether?

@aantron
Copy link
Collaborator

aantron commented Jul 22, 2017

Probably the best is to replace it by some kind of summary of, or reference to, the PPX logging support section https://ocsigen.org/lwt/3.0.0/api/Ppx_lwt#2_Logging.

@aantron
Copy link
Collaborator

aantron commented Jul 22, 2017

It looks good. The only thing I'd recommend is adding an example of a command line for triggering the PPX, i.e. with ocamlfind ... -package lwt.ppx ... (hope I didn't miss it if it's already there). We had the equivalent for Camlp4.

@aantron
Copy link
Collaborator

aantron commented Jul 22, 2017

And don't worry about the 4.06+trunk build failure – that's clearly an upstream problem. I'll move it to allowed_failures later in master.

@aantron
Copy link
Collaborator

aantron commented Jul 22, 2017

(And if you'd like to see more details on that upstream issue, ocaml-ppx/ocaml-migrate-parsetree#19 (comment)).

@bobbypriam
Copy link
Collaborator Author

I've updated the PR with the example to use lwt.ppx on ocamlfind and toplevel, and a reference to Ppx_lwt Logging section.

Sorry for the late commits, timezone issues 🙂

@aantron aantron merged commit b5650de into ocsigen:master Jul 23, 2017
@aantron
Copy link
Collaborator

aantron commented Jul 23, 2017

Looks good. Thanks!

@aantron
Copy link
Collaborator

aantron commented Jul 23, 2017

Checked the generated version online (it's hard to do so ahead of time). It looks good as well.

@bobbypriam
Copy link
Collaborator Author

That's good to hear, and thanks for letting me take this!

@bobbypriam bobbypriam deleted the remove-camlp4-reference branch July 23, 2017 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants