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

API feedback from concrete usage example #7

Closed
glennsl opened this issue Jul 2, 2017 · 3 comments
Closed

API feedback from concrete usage example #7

glennsl opened this issue Jul 2, 2017 · 3 comments
Assignees

Comments

@glennsl
Copy link
Contributor

glennsl commented Jul 2, 2017

open MomentRe;

 let () =
    "March 7 2009 7:30pm EST"
    |> Js.String.replace "EST" "-0500"
    |> (fun s => momentWithFormat s "MMMM D YYYY h:mma Z")
    |> Moment.add duration::(duration 12 `hours)
    |> Moment.defaultFormat
    |> Js.log

I wanted to make a date manipulation example for the bucklescript cookbook, but I think the API currently leaves a bit to desire. I realize most of this is from trying to model it very closely to the js API, but just to get this on the record. The issues I have with it are:

  1. momentWithFormat is a bit of a non-sensical name
  2. momentWithFormat is not in pipe form, so I have to use a lambda function to fit it into the pipeline
  3. momentWithFormats arguments aren't labelled, and so very easy to mix up. Doubly so since it's not in pipe form either (I wasted a ton of time on this, trying to figure out why the date passed in was considered invalid...)
  4. I still find it inconsistent to not have the moment... functions in the Moment module (We've talked about this before though)
  5. Moment.adds duration argument label seems redundant
  6. defaultFormat is not a verb, like most other function names
  7. defaultFormat is also an actual property in moment.js, which is a bit confusing.
@jimexist jimexist self-assigned this Jul 3, 2017
@jimexist
Copy link
Owner

jimexist commented Jul 3, 2017

@glennsl thanks for the great feedback, glad that you actually put it into real use cases

  • for the lack of a better name I used momentWithFormat, but i'm totally open to better alternatives
  • same with defaultFormat: i didn't notice before.

I think I can make changes on both of seem soon by the end of this week. On 4. and 5. let's revisit after the change.

@jimexist
Copy link
Owner

jimexist commented Jul 3, 2017

#4

@glennsl
Copy link
Contributor Author

glennsl commented Jul 7, 2017

Internally the moment constructor is called createLocal, but I'd use different names depending on the variant. monent() can be now (or make, but that's less specific), moment('Jul 7 2017') can be parse, and its variants parseWithFormat etc., and the other construction forms can be fromObject, fromDate, fromUnixTime, fromArray and clone.

defaultFormat could just be formatDefault

@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

2 participants