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

Question: What code conventions to use? #4

Closed
glennsl opened this issue Feb 27, 2017 · 14 comments
Closed

Question: What code conventions to use? #4

glennsl opened this issue Feb 27, 2017 · 14 comments

Comments

@glennsl
Copy link
Member

glennsl commented Feb 27, 2017

  1. Should names be js-y (forEach) or ml-y (for_each)? Reason is going for js-y, and the npm ecosystem is obviously js-y, yet many bindings seem to go for ml-y naming.
  2. What about file naming? BS has some constraints here, e.g. no dasherized filenames allowed.
  3. Standard prefix or postfix for module signatures? E.g. IModule, Module_Sig or MODULE
  4. Should types follow the same convention as functions and variables, or be different?
  5. Constants?
  6. Prefer short names for brevity or long names for clarity?
  7. Should comments be placed before or after the code they address.
  8. Tabs or spaces? 2 or 4 spaces per indent level?
  9. Prefer piping or intermediate let bindings?
  10. Prefer labeled and optional arguments when several arguments have same type? Append a unit argument when all arguments are labeled?
  11. Prefer let f : int -> string = fun x ->... over let f (x: int): string = ...?
  12. Place vertical bar | indented or leveled with the enclosing structure in variant defintions, match expressions and piping?
  13. How to break long lines?
  14. Exit branches first or last in a pattern match?
  15. Named convention for "overloading" externals (Question: What code conventions to use? #4 (comment))
  16. Naming convention for list destructuring (Question: What code conventions to use? #4 (comment))
@chenglou
Copy link
Contributor

The last two are solved by refmt; no need to establish conventions?

@glennsl
Copy link
Member Author

glennsl commented Feb 28, 2017

refmt isn't even consistent with itself though

This is how it formats ml and reason, respectively:

let _ =
  match Background.Refmt.refmt input with
  | ("Failure",error) -> cb error None None
  | (conversion,outText) ->
      (match conversion |> (Js.String.split "to") with
       | [|inLang;outLang|] -> cb outText (Some inLang) (Some outLang)
       | _ -> ())
type 'a case =
  | StrictEqual of 'a* 'a
  | NotStrictEqual of 'a* 'a
  | Equal of 'a* 'a
  | NotEqual of 'a* 'a
switch (Background.Refmt.refmt input) {
| ("Failure", error) => cb error None None
| (conversion, outText) =>
  switch (conversion |> Js.String.split "to") {
  | [|inLang, outLang|] => cb outText (Some inLang) (Some outLang)
  | _ => ()
  }
};

type case 'a =
  | StrictEqual 'a 'a
  | NotStrictEqual 'a 'a
  | Equal 'a 'a
  | NotEqual 'a 'a;

And chained pipe operators is pretty broken:

But the convention and what refmt does SHOULD of course be the same.

@chenglou
Copy link
Contributor

We can't do much for the ocaml formatter; we currently don't control/have the energy to make it work well (plus, it doesn't even preserve comments). Pipe being broken is indeed bad, but it should be considered as a refmt bug. Formatting conventions should be filed to the reason repo as issues.

@glennsl
Copy link
Member Author

glennsl commented Feb 28, 2017

Sorry, I didn't mean for this to be an issue to discuss and establish every detail of convention, but rather to point out some frequently asked questions that ought to be answered by an FAQ. And as you point out, refmt isn't going to provide an answer for BuckleScript in general, just Reason specifically.

@chenglou
Copy link
Contributor

hey no need to be sorry, I was trying to be mean or anything lol

@glennsl
Copy link
Member Author

glennsl commented Feb 28, 2017

(Sorry,) I might be part canadian (I'm kind of in the right latitude at least)

@chenglou
Copy link
Contributor

Sorry, I'm Canadian too

@chenglou
Copy link
Contributor

chenglou commented Mar 1, 2017

Ok let's see

  1. For Reason work, let's go with camelCase. As an aside, I haven't seen people complain about this too much, which is nice.

  2. No dash, no space. camelCase (just to be consistent with above). First letter must be uncapitalized to avoid case-sensitive FS problem. This needs to be encoded in the build system/tools, somehow: Upper-cased files compiled inconsistently on Linux/OS X rescript-lang/rescript-compiler#913 lots of people will skip this wiki.

  3. No idea. Let's not establish this one yet.

  4. Yeah.

  5. justLikeThisSinceEverythingIsConstantByDefault

  6. Not sure yet.

  7. Not sure yet either. Should be solved through refmt: New attribute comment idea reasonml/reason#1015

  8. refmt

  9. Not sure either.

  10. Yes, prefer that. For appending unit: necessary for optional (will warn anyway; we'll just provide a good message like in BetterErrors). No unit needed for non-optional labelled functions otherwise.

  11. Those two examples happen to be the same. In general they're not, so they need two separate AST representations. The former allows room for polymorphic type variables.

  12. refmt

  13. refmt

Rough draft of answer. Do the answers make sense?

@glennsl
Copy link
Member Author

glennsl commented Mar 2, 2017

Regarding 10. Non-optional labeled functions doesn't need an extra unit argument, but @bobzhang once told me it's an OCaml convention to do so still. I don't know why, but I've been doing that for the Js FFI. So should it be preferred? (And if so there should be some reasoning behind it)

11: Same question, when the two forms are equivalent, which should be preferred?

@glennsl
Copy link
Member Author

glennsl commented Mar 4, 2017

Another convention question: Should the "exit" branch(es) of a pattern match come first or last? E.g.

match get_opt foo with
| Some value ->
  (* do something with value
   * over several lines
   *)
| None -> ()

vs.

match get_opt foo with
| None -> ()
| Some value ->
  (* do something with value
   * over several lines
   *)

The latter is clearer, especially if there's nested matches, but sometimes the "exit" branches will be a wildcard match, in which case it has to come last. Should that then be the convention just for consistency, or should it be contextual?

@chenglou
Copy link
Contributor

chenglou commented Mar 4, 2017

None isn't an exit, and sometimes the None branch is the spanning multiple lines. I'd say let's not impose a convention for this one yet.

@glennsl
Copy link
Member Author

glennsl commented Mar 4, 2017

Yea I'm not saying None is "an exit", just using it as an example of a branch that "exits".

This is somewhat similar to the imperative dilemma:

if (thing) {
  /* do things with thing */
}

vs.

if (!thing) {
  return;
}
/* do things with thing */

(I prefer the latter for imperative code)

I've no problem with not instantly getting this answered though, I'm just putting it somewhere for safekeeping :)

@ul
Copy link

ul commented Mar 24, 2017

  1. What about names of external instances for multi-type-with-optional-args functions, like https://nodejs.org/dist/latest-v7.x/docs/api/buffer.html#buffer_buf_copy_target_targetstart_sourcestart_sourceend ? My attempts ended with long unpronounceable (huh, this word itself is such one) suffix-piles. Thanks to @glennsl they are better now, but good to have smth. like explicit conventions for consistency and reduced mental exercises when write every multi-external binding.

@glennsl
Copy link
Member Author

glennsl commented Jun 1, 2017

Another one to ponder: There are several conventions among functional programmers for how to name variables when destructuring a list:

  • head::tail, with the short-form h::t
  • first::rest
  • x::xs (and y::ys etc.)
  • The ocaml stdlib seems to be using a::l and a1::l1, a2::l2 etc.

We should probably try to standardize on one of these.

@glennsl glennsl closed this as completed Mar 24, 2018
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

No branches or pull requests

3 participants