Skip to content

Initial #1

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

Merged
merged 10 commits into from
Jul 21, 2016
Merged

Initial #1

merged 10 commits into from
Jul 21, 2016

Conversation

cryogenian
Copy link
Member

There is no real tests, just couple checks that this works. I'll add them during work on purescirpt-echarts update.

Fixes slamdata/slamdata#954

@garyb Please review

@garyb
Copy link
Member

garyb commented Jul 20, 2016

I'm not at my desk right now so will review properly when I get home, but just from skimming on my phone this looks cool!

From a meta point of view, maybe we should break these up into separate repos though? Date and number formatting don't have much in common together, even though date formatting probably depends on number formatting.

map f (Placeholder str a) = Placeholder str $ f a
map f End = End

type Formatter = Mu FormatterF
Copy link
Member

Choose a reason for hiding this comment

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

What's the idea with using Mu here, rather than something like newtype Formatter = List FormatterF? I mean, it definitely works! But it wouldn't have occurred to me to use Mu for any non-tree structure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I don't know why I chosed Mu rather then List. That was my first intent and it works, so I didn't even try List

@garyb
Copy link
Member

garyb commented Jul 21, 2016

We could probably do with adding some documentation explaining the options for the format strings. The date ones I'm familiar with already, so had no problem there, but I'm having difficulty figuring out how the number format patterns actually work: does it matter where the comma appears, for instance, and what does "after" and "before" refer to with that? Places before/after the decimal point?

I'm not sure the numeric formats are actually working correctly either:

> FN.formatNumber "0.0" 123345.1235
Right { value0: '123,345.1' } -- is the comma supposed to be inserted?

> FN.formatNumber "0.0" (-123345.1235)
runtime exception! (unsafePartial'ed Maybe pattern failure)

@cryogenian
Copy link
Member Author

  • Fixed number format error
  • Two digit year now checks if it's more then 69 and if so think that it's in 20th century otherwise in 21st.
  • Changed to 2-digit
  • Handling of pm + hour > 12

I think it's ok to have both formatters in one library because

  • They are both formatters
  • They use same libs
  • If one wants formatting numbers we could be almost sure that he/she wants formatting datetimes and viceversa.


Formatter has following properties
+ Number of digits before dot
+ Numberr of digits after dot
Copy link
Member

Choose a reason for hiding this comment

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

Minor typo: "numberr" 😄

@cryogenian
Copy link
Member Author

Updated readme

then use formatter to result of division of input number and that power.
+ `0a`: `1234567 -> "1M"`, `1 -> "1"`

## Datetim formatters
Copy link
Member

Choose a reason for hiding this comment

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

"Datetim" -> "Date/Time"? 😄

@garyb
Copy link
Member

garyb commented Jul 21, 2016

When the tests pass this time (:wink:) :+1: from me. I'll hook up the dependency badges and stuff like that before making a release though, so if you just merge this I can make the actual release shortly after, if that's good with you?

@cryogenian
Copy link
Member Author

Yep, I was thinking about making something like 0.0.1 for this and then check what's actually needed by echarts, if there are any bugs etc and then switch to 0.1.0.

Thanks for review!

@cryogenian cryogenian merged commit 95df29e into purescript-contrib:master Jul 21, 2016
@cryogenian cryogenian assigned cryogenian and unassigned garyb Jul 21, 2016
@garyb
Copy link
Member

garyb commented Jul 21, 2016

Ah good point! Go ahead with the release then and I'll get the other stuff in place for the bumped version release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants