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

Add type level format builder #27

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

safareli
Copy link
Contributor

fix #22

@safareli
Copy link
Contributor Author

safareli commented Jul 15, 2017

Was playing with this staff and this is what I came to.
Need to:

  • add Date checks
  • update README
  • some cleanup

@cryogenian
Copy link
Member

Could you add some examples please?

Copy link
Member

@garyb garyb left a comment

Choose a reason for hiding this comment

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

I'm not sure we necessarily need all the checks here, we mainly are trying to prevent broken/ambiguous formats, not stupid ones that have duplicate minutes or something. Just thinking about that as every check we add slows down the compilation time!

, ValidSecondsType secondsOut
, ValidateMilliseconds a NoMilliseconds millisecondsOut
, ValidMillisecondsType millisecondsOut
) ⇒ ValidTimeI a
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call this ValidTimePart?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

) ⇒ ValidDate a

class ValidDateI (a ∷ FormatList)
instance vdi ∷ ValidDateI a
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 going on here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not implemented it date staff yet.
Here needs to go minute and seccond like checks for year month ....

instance rdi18 ∷ PrintFormatI rest ⇒ PrintFormatI (SecondsTwoDigits : rest) where printFormatI _ acc = printFormatI (FProxy ∷ FProxy rest) (flip Cons acc FC.SecondsTwoDigits)
instance rdi19 ∷ PrintFormatI rest ⇒ PrintFormatI (Milliseconds : rest) where printFormatI _ acc = printFormatI (FProxy ∷ FProxy rest) (flip Cons acc FC.Milliseconds)
instance rdi20 ∷ PrintFormatI rest ⇒ PrintFormatI (MillisecondsShort : rest) where printFormatI _ acc = printFormatI (FProxy ∷ FProxy rest) (flip Cons acc FC.MillisecondsShort)
instance rdi21 ∷ PrintFormatI rest ⇒ PrintFormatI (MillisecondsTwoDigits : rest) where printFormatI _ acc = printFormatI (FProxy ∷ FProxy rest) (flip Cons acc FC.MillisecondsTwoDigits)
Copy link
Member

Choose a reason for hiding this comment

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

No need to flip these 😄. Plus you can compose I think:

printFormatI _ = printFormatI (FProxy  FProxy rest) <<< Cons FC.YearFull

etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was late when i did that :d

Copy link
Contributor Author

@safareli safareli left a comment

Choose a reason for hiding this comment

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

I'm not sure we necessarily need all the checks here, we mainly are trying to prevent broken/ambiguous formats, not stupid ones that have duplicate minutes or something. Just thinking about that as every check we add slows down the compilation time!

For stupid formats one could just build value level list and wrap in Unsafe.x.Format

Is it, that big of an issue?


I think we should also add such checks for values. like in case someone wants to validate dynamic format entered by a user.

) ⇒ ValidDate a

class ValidDateI (a ∷ FormatList)
instance vdi ∷ ValidDateI a
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not implemented it date staff yet.
Here needs to go minute and seccond like checks for year month ....

, ValidSecondsType secondsOut
, ValidateMilliseconds a NoMilliseconds millisecondsOut
, ValidMillisecondsType millisecondsOut
) ⇒ ValidTimeI a
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

instance rdi18 ∷ PrintFormatI rest ⇒ PrintFormatI (SecondsTwoDigits : rest) where printFormatI _ acc = printFormatI (FProxy ∷ FProxy rest) (flip Cons acc FC.SecondsTwoDigits)
instance rdi19 ∷ PrintFormatI rest ⇒ PrintFormatI (Milliseconds : rest) where printFormatI _ acc = printFormatI (FProxy ∷ FProxy rest) (flip Cons acc FC.Milliseconds)
instance rdi20 ∷ PrintFormatI rest ⇒ PrintFormatI (MillisecondsShort : rest) where printFormatI _ acc = printFormatI (FProxy ∷ FProxy rest) (flip Cons acc FC.MillisecondsShort)
instance rdi21 ∷ PrintFormatI rest ⇒ PrintFormatI (MillisecondsTwoDigits : rest) where printFormatI _ acc = printFormatI (FProxy ∷ FProxy rest) (flip Cons acc FC.MillisecondsTwoDigits)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was late when i did that :d

@safareli
Copy link
Contributor Author

safareli commented Aug 5, 2017

@cryogenian example:

type MyFormat = MonthShort : YearFull : DayOfMonthTwoDigits : FNil

myFormat :: Formatter
myFormat = printFormat (FProxy :: FProxy MyFormat)

@safareli
Copy link
Contributor Author

safareli commented Aug 6, 2017

Typelevel checks are done at this point, except DayOfWeek which I think should be removed and introduced when we fully support it #28.

TODO:

  • Add value level checks for validating dynamic formats.
  • Add tests for typelevel validation
  • reorder namespaces and code cleanup
  • Update README: add examples of typelevel and non string formats

@cryogenian cryogenian removed their request for review July 1, 2019 11:52
@thomashoneyman thomashoneyman changed the base branch from master to main October 4, 2020 01:29
@JordanMartinez
Copy link
Contributor

I don't think this really adds anything to this repo. Closing.

@safareli
Copy link
Contributor Author

From previous discussion I think it's clear that:

  • Other contributors of this lib at that time haven't considered that it was worthless.
  • This is 4 years old PR that's definitely lacking activity from the original contributor.

Even if this pr was total crap, "Closing due to lack of activity" would have been more accurate as well as respectful reason for closing.

@thomashoneyman
Copy link
Contributor

Sorry, @safareli, you’re completely right that that is a more accurate and respectful reason to close this. I would guess that @JordanMartinez was in a hurry as he’s working on a bunch of libraries at the same time right now and didn’t think about how those words would come across. Thank you for raising an objection, as we do want to make sure past, current, and future contributors feel respected.

@safareli
Copy link
Contributor Author

safareli commented Sep 20, 2021

If you think this is worthless then issue this PR was addressing is probably a good candidate to close due to being worthless :)

@garyb garyb reopened this Sep 20, 2021
@JordanMartinez
Copy link
Contributor

@safareli I'm very sorry. My tone was completely disrespectful. I didn't take the time to really understand what this PR was trying to solve. Yes, it would have been better to say that I was closing this due to inactivity.

For context, we have 237 open PRs across core, contrib, web, and node libraries. I was doing a quick check to see where the state of such PRs were. If the changes were small and I could finish them, then I would. Otherwise, my default was to close them because nothing had been done for a long time.

@safareli
Copy link
Contributor Author

I'm not actively doing PS any more (for now), so this could be closed. (if anyone is up for taking this on they can just checkout from this branch or copy paste code)

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.

Update readme/docs to de-emphasise usage of string formats
5 participants