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

Add command line argument for generating reason #150

Merged
merged 9 commits into from Aug 5, 2021

Conversation

outkine
Copy link
Contributor

@outkine outkine commented Aug 3, 2021

Closes #95

Copy link
Owner

@aantron aantron left a comment

Choose a reason for hiding this comment

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

Thanks!

The majority of the comments I left are either nits or have to do with the example text. Feel free to ignore those, as I often touch up example text after examples are merged anyway (whether written by me or others). The important comments are under src/eml/.

src/eml/main.ml Outdated Show resolved Hide resolved
src/eml/main.ml Outdated Show resolved Hide resolved
src/eml/main.ml Outdated
@@ -7,7 +7,7 @@

module Command_line :
sig
val parse : unit -> (string * string) list
val parse : unit -> (string * string * bool) list
Copy link
Owner

Choose a reason for hiding this comment

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

With the way the code is currently written, there is a global syntax flag for all the files, so the result type should be

(string * string) list * bool

However, I think it would be even better to move the code from process_file in eml.ml, which checks the extension, into the command-line parser, and decide here what syntax each file will use. Then, the result type should be either what this PR has now, or, even more helpfully

(string * string * [ `OCaml | `Reason ]) list

example/r-template-files/template.eml.html Outdated Show resolved Hide resolved
example/r-template-files/README.md Outdated Show resolved Hide resolved
example/r-template-files/README.md Show resolved Hide resolved
example/r-template-files/README.md Outdated Show resolved Hide resolved
example/r-template-files/README.md Outdated Show resolved Hide resolved
example/r-template-files/README.md Outdated Show resolved Hide resolved
example/r-template-files/README.md Outdated Show resolved Hide resolved
Move logic that decides whether to use Reason into the cli, and pass
this information as a variant.
@outkine
Copy link
Contributor Author

outkine commented Aug 4, 2021

Thank you for the exhaustive feedback, I completely forgot to update the new README.md.

@aantron
Copy link
Owner

aantron commented Aug 4, 2021

Please don't resolve any conversations startred by the reviewer — it's for the reviewer to keep track of those conversations, and whether whatever issues are in them have been addressed. Otherwise, it can get hard for the reviewer to keep track of what has actually been "resolved" and what hasn't. In general, as a first approximation, the person who started the conversation decides if/when it is resolved.

src/eml/main.ml Outdated Show resolved Hide resolved
@aantron aantron merged commit 120fc9f into aantron:master Aug 5, 2021
@aantron
Copy link
Owner

aantron commented Aug 5, 2021

Many thanks!

@outkine outkine deleted the eml-templater-reason-argument branch August 5, 2021 05:27
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.

Explicitly setting Reason mode in the eml templater
2 participants