reasonml / reason

Simple, fast & type safe code that leverages the JavaScript & OCaml ecosystems
http://reasonml.github.io
MIT License
10.14k stars 428 forks source link

Mixing camelCase and snake_case is aesthetically off-putting #1689

Closed gaearon closed 6 years ago

gaearon commented 6 years ago

This is a very subjective opinion but I figured I’d raise this. There was a past issue raised as a question (https://github.com/facebook/reason/issues/1362). However, my issue is not a question but an observation about something that might impede Reason's adoption.

As a JS developer, there’s one thing that always brings out a knee-jerk aversion to the Reason code in me, and that's the way camelCase and snake_case identifiers are mixed in the code. I understand why this is the case (OCaml built-ins vs userland functions). But I just can't get past it. The inconsistency makes Reason feel a second-class citizen. In fact I’m surprised by how strongly I feel about this.

Is there any chance this might get reconsidered, and the style could get unified? There is precedent for that. For example, when Xamarin created C# bridges to iOS API, they (mostly) automatically created (typed) bindings with .NET-friendly PascalCase convention. Couldn't Reason do something similar for OCaml APIs?

It wouldn’t have been as pressing for me if snake_case was only used in rare interop cases. In fact that would be helpful to highlight “dangerous” code. It doesn’t have that connotation though. I recognize this is a petty complaint. But “this looks weird” is an impediment to adoption whether we agree with it or not.

  and renderComment = ({ ReasonReact.state } as self, id: int) => {
    let commentMaybe = JSMap.get(story.comments, id);

    <div key=(string_of_int(id))>
      (
        switch commentMaybe {
        | Some(commentPresentOrDeleted) =>
          switch commentPresentOrDeleted {

          | StoryData.CommentPresent(comment) =>
            let openComment = ! JSSet.has(state.collapsed_comments, comment.id);
            <div className="CommentList_comment">

              <div className="CommentList_disclosureRow CommentList_inline"
                   name=(string_of_int(comment.id))
                   onClick=(self.reduce(event => Toggle(getCommentIdFromEvent(event))))>
petetnt commented 6 years ago

I have been doing ReasonML things for a week or so now and this is one of the things that trips me quite often. It's something that's not only aesthetically off-putting, but it also increases cognitive load when nearly everything else is either camelCase or PascalCase. int_of_string or intOfString or IntOfString, Array.fold_left or Array.foldLeft etc. Sure, you get used to it, but it does make learning harder.

Not only that, there are also things like exception Not_found which is basically all of cases combined.

strega-nil commented 6 years ago

I'm still on the snake_case + Upper_snake_case bandwagon.

jordwalke commented 6 years ago

I agree. Some OCaml libraries publish two versions of their modules in their released package. One version has labeled arguments and the other has name-less arguments. We could encourage people to do the same but for snake/camel (if they insist on having snake), but I think the largest impact would be to merely have a facade over the standard library that reexports everything in the standard library, but keeps the same implementation (that's where string_of_int comes from btw).

I remember when, at Facebook, snake_case was the standard convention for JS code (if you're curious, I pretty much just disregarded the convention and wrote camelCase). Now, camelCase has clearly won out in JS (inside and outside of Facebook). These conventions can change fast.

In your own Reason code, I recommend just using camelCase since that will be more familiar to JS developers (and it saves horizontal real estate preventing line wraps!)

bobzhang commented 6 years ago

@gaearon I am working on a new stdlib, it will use camlCase consistently

chenglou commented 6 years ago

Done. New stdlib landed in bs-platform 2.2.2

gaearon commented 6 years ago

Wow! Is there anywhere I can see before/after comparisons?

chenglou commented 6 years ago

Deja vu =P

Yeah sure, any of my recent diff in reasonml-community. Example: https://github.com/reasonml-community/reason-maze/commits/master

(Beta API for the new stdlib stuff; disregard a few spots that look confusing for now). No string_of_int conversion on particular yet since that was never the idiom for the JS compilation. You'd have used string interpolation.

manu-unter commented 5 years ago

This seems to be the hard-to-find but official documenation for Belt, which is the JS-oriented wrapper for the stdlib:

https://bucklescript.github.io/bucklescript/api/Belt.html

Even though we have this now, I'd like to revisit the topic of automatically creating bindings in consistent casing. The reason for that is that my current use-case won't be covered by Belt. We are writing native platform oriented Reason in revery which is not build on BuckleScript. That means we only have the regular stdlib.

Now for this and maybe other use-cases where existing OCaml libraries should be used from Reason code, I think it would make sense to think about an automatic way to create camelCased bindings from them.

Is there anyone else around that would be willing to explore this topic with me? I'm fairly new to writing compilers, so I'd probably need some help figuring this out.

jordwalke commented 5 years ago

@cryza A couple of other options: you could provide camel cased bindings to Base as an alternative for native, or encourage someone to release Belt as a native package with native tests etc. For automatic bindings, there needs to be a way to express every possible AST form. So if an AST with xyz_abc is expressed in Reason as xyzAbc then we'd need to come up with a way in Reason to represent the AST with xyzAbc.

manu-unter commented 5 years ago

you could provide camel cased bindings to Base as an alternative for native, or encourage someone to release Belt as a native package with native tests etc.

I think @jaredly even already found a way to run Belt natively in reason-language-server. I haven't gotten a chance yet to investigate how he did it though. Promoting Belt as the stl replacement for Reason is a pretty reasonable thing to do I think. If I understood correctly, the ocaml compiler will by default open the regular stl into every file, right? Could it be a good idea to change that in favor of Belt by default at some point in the future?

For automatic bindings, there needs to be a way to express every possible AST form. So if an AST with xyz_abc is expressed in Reason as xyzAbc then we'd need to come up with a way in Reason to represent the AST with xyzAbc.

That is also what's bothering me about the completely automatic approach. Maybe it would be more reasonable to offer an easy way of generating a camelCased Reason wrapper for an existing OCaml module then? You would then generate a wrapper for whatever library you are using and if the generator finds conflicts, you would have the opportunity to decide what to do. We could also emit a warning on compile time that suggests creating such a wrapper if we find snake_cased bindings somewhere?

wegry commented 5 years ago

Aliasing the conversion functions (string_of_int, string_of_bool, etc.) to have camelCase versions would be the last part of removing snake_case from reason right? Or does something similar already exist?

manu-unter commented 5 years ago

@wegry Currently, there is no canonical native binding for Belt, which is why the Belt solution only works as long as you're targeting JavaScript for now. In all use-cases that target native compilation, we have to fall back to the snake_cased OCaml stdlib (Or do whatever reason-language-server does to get Belt running natively).

If we manage to create off-the-shelf native bindings for Belt and if Belt also offers enough functionality to completely replace the stlib, then I think you're right.

On the string_of_int etc. topic: I think Belt purposely left those transformation functions out because the idiomatic way of doing that would apparently be using string interpolation. I'm not sure if that holds also for non-React-related use-cases but I'm sure this is up for discussion.

jaredly commented 5 years ago

string_of_X are idiomatic, but Belt has purposely not (yet) included any String module or string-handling functions because utf-8 etc. are notoriously difficult to get right.