Skip to content

Add string delimiter compatibility wrapper. #36

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

Merged
merged 1 commit into from
Apr 1, 2016
Merged

Add string delimiter compatibility wrapper. #36

merged 1 commit into from
Apr 1, 2016

Conversation

aantron
Copy link
Contributor

@aantron aantron commented Mar 31, 2016

@alainfrisch
Copy link
Collaborator

Ok in principle, but the name is not very explicit. What about quotation_delimiter, as used in Ast_helper.Const.string?

@Drup
Copy link
Collaborator

Drup commented Mar 31, 2016

I think we should rather add a get_full_str that would return both the string and the delimiter. You usually want both at the same time and the multiple option wrapping are a bit annoying.

@aantron
Copy link
Contributor Author

aantron commented Mar 31, 2016

Will change it to whatever we agree on. I'm fine with changing the name and combining with retrieving the string, but what do you think @alainfrisch?

@alainfrisch
Copy link
Collaborator

My proposal:

val get_str_with_quotation_delimiter: expression -> (expression * string option) option

The name is long, but more explicit than get_full_str; and we don't want to encourage too much ppx authors to depend on the quotation delimiter (which, in its original design at least, is supposed to be purely syntactic).

@Drup
Copy link
Collaborator

Drup commented Apr 1, 2016

Works for me.

@aantron
Copy link
Contributor Author

aantron commented Apr 1, 2016

Updated, assuming you meant (string * string option) option for the result type.

@alainfrisch alainfrisch merged commit 0ea44f4 into ocaml-ppx:master Apr 1, 2016
@aantron aantron deleted the delimiter-4.03 branch April 2, 2016 17:11
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