ocaml-sys / config.ml

conditional compilation via attributes for OCaml
Other
27 stars 3 forks source link

Feature: Add cases and function branch #13

Closed KFoxder closed 5 months ago

KFoxder commented 6 months ago

Overview

TODO

KFoxder commented 6 months ago

@leostera & @gpetiot I have a couple questions before I think its ready for review:

Styling: What is the best way for a variable naming convention in OCaml. I did take a look at https://ocaml.org/docs/guidelines but didn't see anything useful. Particularly I am asking around shadowing variable names like exp in the below example. I am aware it is "correct" in that the scoping works but just not confident that it is named as best it could be. How would you name the variables there?

let rec apply_config_on_expression (exp : expression) =
  let pexp_desc =
    match exp.pexp_desc with
    | Pexp_try (exp, cases) ->
        let exp = apply_config_on_expression exp in
        let cases = apply_config_on_cases cases in
        Pexp_try (exp, cases)

Logic:

  1. I didn't try to address every single case for expression_desc. I only addressed the ones I needed to add the use cases described in the issues. I definitely can add them all and flesh it out, but before I did wanted to take a pause and get your feedback of whether I should in this PR or not.
    • If they aren't added it could lead to situations where someone would expect it to work with the following code and it doesn't:
      (* Won't work because I am not covering the `Pexp_setfield` *)
      a.prop <- (match x with | "0" -> 0 | "1" -> 1 [@cfg (target_ox = "macos")])
  2. The apply_config_on_expression function right now is just concerned with removing cases and not concerned with removing entire expressions. Therefore I am not returning an Option monad. I think in the future if the @cfg is applied to an expression and we want it to remove the entire expression we may want to do that but felt like overkill for now. Thoughts?
KFoxder commented 5 months ago

@leostera this can be reviewed when you get a chance. Not a priority or rush though.

KFoxder commented 5 months ago

@leostera Thanks for the PR review on a Sunday! Hope you are enjoying NYC!

leostera commented 5 months ago

Thanks for the PR @KFoxder! ✨