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

Reason distillery #504

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

charlesetc
Copy link
Member

This is building off of #445 by @vasilisp.

It's adding the -reason flag to eliom-distillery which converts the .eliom files to reason syntax. It also adds the %%%REASON_FLAG%%% substitution which evaluates to "-reason" if -reason is passed to eliom-distillery and an empty string otherwise.

Hopefully this gets ocsigen closer to reason support - I haven't been able to get it working with ocsigen-start, but this does generate a reason ocsigen program with:

eliom-distillery -name project_name -template basic.ppx -reason

The problems current with getting ocsigen start to work are in the translation from the ocaml to reason.

  • There is some camlp4 for postgres. This only affects 2 files. And there are some ppx forks of pgocaml but they aren't on opam
  • The syntax ##. doesn't seem to work when converting from ocaml to reason, it's used in js_of_ocaml ppx

It's possible these problems are too great right now - we might want to wait to merge the -reason flag until there's support for ocsigen start. Either way, it's a little closer to reason and ocsigen! ✈️

vasilisp and others added 8 commits June 15, 2017 16:09
When using refmt, we end up with intermediate filenames that break
our identifier generation.
This reverts commit 48fab60.
The reason is that we can now generate a basic reason
template with `-reason -template basic.ppx`
@charlesetc charlesetc requested a review from vasilisp June 16, 2017 14:52
@Drup
Copy link
Member

Drup commented Jun 16, 2017

I'm surprised by the ##. issue. Are reason people aware of it ?

Copy link
Contributor

@vasilisp vasilisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I have no comments on the code. Looks good.

I like the idea of using refmt during template instantiation.

I am a bit worried about the template/eliom-distillery interface. If we were to add a template using Ocamlbuild/Jbuilder/..., would we be able to interpret your fixed %%%REASON_FLAG%%%? Could this be customized? Or at least, could we allow individual templates to declare support for Reason (or lack thereof)?

@vasilisp vasilisp mentioned this pull request Jun 27, 2017
@charlesetc
Copy link
Member Author

Hmm that's a good point about the -reason flag being specific to eliomc, etc. I'm not sure how to customize it ... maybe we could change it to REASON_ELIOMC_FLAG to be more specific, or as you say declaring support for reason in the templates makes sense. What do you think?

@vasilisp
Copy link
Contributor

vasilisp commented Jul 3, 2017

Sorry for the delay.

The customizability of the flag and the declaration of Reason support could both be implemented via a per-template configuration/specification file. It could be useful for other features (e.g., supersede the existing.eliom* files), and we can discuss it later.

For now, could you work-around the use of Filename.extension via Filename.check_suffix (or otherwise)? Let's not break 4.03 compatibility just yet.

Thanks!

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.

3 participants