Mapped(Date)(Time) formatter/parser

[Naftoli Gugenheim]

David and all,
QUESTION 1
I'm working on issue #258. Here are two options for an overridable parser (applies to formatting too):
1. def parse(s: String): Box[Date] = ConversionRules.parseDate()(s)
2. def parse: String=>Box[Date] = ConversionRules.parseDate()

What are the pros and cons, and which should I use?

QUESTION 2
MappedDate and MappedDateTime apparently were parsing via LiftRules in setFromAny, while buildSetStringValue etc. were using (Time)Helpers.toDate. Is there a reason not to change it? Or, should toDate use ConversionRules?

Thanks.

[Jeppe Nejsum Madsen]

Naftoli Gugenheim <nafto...@gmail.com> writes:

> David and all,
> QUESTION 1
> I'm working on issue #258. Here are two options for an overridable parser (applies to formatting too):
> 1. def parse(s: String): Box[Date] = ConversionRules.parseDate()(s)
> 2. def parse: String=>Box[Date] = ConversionRules.parseDate()
>
> What are the pros and cons, and which should I use?

I prefer 1) I see no reason for parse to be a function....

> QUESTION 2
> MappedDate and MappedDateTime apparently were parsing via LiftRules in setFromAny, while buildSetStringValue etc. were using (Time)Helpers.toDate. Is there a reason not to change it? Or, should toDate use ConversionRules?

I think it would be natural for toDate to use ConversionRules....then
ConversionRules becomes the only place where date formatting is handled.

/Jeppe

[Indrajit Raychaudhuri]

On 05/02/10 5:28 PM, Jeppe Nejsum Madsen wrote:
> Naftoli Gugenheim<nafto...@gmail.com> writes:
>
>> David and all,
>> QUESTION 1
>> I'm working on issue #258. Here are two options for an overridable parser (applies to formatting too):
>> 1. def parse(s: String): Box[Date] = ConversionRules.parseDate()(s)
>> 2. def parse: String=>Box[Date] = ConversionRules.parseDate()
>>
>> What are the pros and cons, and which should I use?
>
> I prefer 1) I see no reason for parse to be a function....

+1

[Naftoli Gugenheim]

In reverse order:
Re #2, (a) that would create a two-way dependency between TimeHelpers and ConversionRules--is that desirable; (b) and thus call into question the decision to put ConversionRules in webkit not util. (c) Still, it would be an indirect branch from MappedDate etc. to ConversionRules--is there a reason?
Re #1, I'm thinking maybe an even better idea is instead signatures like
def format: String
def parse(s: String): Unit
In other words, they should deal directly with the field's state.
The advantage is that it provides a future hook to deal with parsing errors similar to validation errors.
Which brings me to two somewhat unrelated quesions.
1) What is the reason for 'setFromAny'? Why not overloaded methods?
2) Parsing dates with a DateFormat seems to be very brittle -- the string has to match the format exactly, e.g., if you write 1:00PM and the format string has a space before the a, it's invalid, etc.--not very user friendly. Is there a better way?
-------------------------------------

Jeppe Nejsum Madsen<je...@ingolfs.dk> wrote:

Naftoli Gugenheim <nafto...@gmail.com> writes:

> David and all,
> QUESTION 1
> I'm working on issue #258. Here are two options for an overridable parser (applies to formatting too):
> 1. def parse(s: String): Box[Date] = ConversionRules.parseDate()(s)
> 2. def parse: String=>Box[Date] = ConversionRules.parseDate()
>
> What are the pros and cons, and which should I use?

I prefer 1) I see no reason for parse to be a function....

> QUESTION 2
> MappedDate and MappedDateTime apparently were parsing via LiftRules in setFromAny, while buildSetStringValue etc. were using (Time)Helpers.toDate. Is there a reason not to change it? Or, should toDate use ConversionRules?

I think it would be natural for toDate to use ConversionRules....then
ConversionRules becomes the only place where date formatting is handled.

/Jeppe

--
You received this message because you are subscribed to the Google Groups "Lift" group.
To post to this group, send email to lif...@googlegroups.com.
To unsubscribe from this group, send email to liftweb+u...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/liftweb?hl=en.

[Naftoli Gugenheim]

Any thoughts on this?
Basically, to restate the questions in a different way.
- I would like to add overridable methods to MappedDate/Time/DateTime, to allow controlling the way they parse and format strings.
- Do I need a new 'format' method to controll conversion to a string? Maybe just leverage toString?
- Until now there were two places in Mapped(Date)(Time) where parsing occurred. setFromAny used LiftRules.parseDate, while buildSetStringValue etc. used TimeHelpers.toDate.
- Do we need methods like setFromAny and TimeHelpers.toDate, which take an Any and pattern match on several types of inputs? Isn't that very un-typesafe?
- Why the inconsistency between setFromAny and buildSetStringValue? Should both use ConversionRules? Should both use toDate which should use ConversionRules?
- Should anything in TimeHelpers use ConversionRules? (After all, ConversionRules uses TimeHelpers, and two-way dependencies are probably undesirable.) Should ConversionRules be in util rather than webkit?

Thanks!

-------------------------------------

[Jeppe Nejsum Madsen]

Naftoli Gugenheim <nafto...@gmail.com> writes:

> Any thoughts on this?
> Basically, to restate the questions in a different way.
> - I would like to add overridable methods to MappedDate/Time/DateTime, to allow controlling the way they parse and format strings.
> - Do I need a new 'format' method to controll conversion to a string? Maybe just leverage toString?

I think toString is fine

> - Until now there were two places in Mapped(Date)(Time) where parsing occurred. setFromAny used LiftRules.parseDate, while buildSetStringValue etc. used TimeHelpers.toDate.

Should use same mechanism

> - Do we need methods like setFromAny and TimeHelpers.toDate, which take an Any and pattern match on several types of inputs? Isn't that very un-typesafe?
> - Why the inconsistency between setFromAny and buildSetStringValue? Should both use ConversionRules? Should both use toDate which should use ConversionRules?
> - Should anything in TimeHelpers use ConversionRules? (After all, ConversionRules uses TimeHelpers, and two-way dependencies are probably undesirable.) Should ConversionRules be in util rather than webkit?

I don't know why things are as they are, but going forward, my assumption is
this:

- If I change date/time parsing in ConversionRules, this changes all the
places where "default" date/time parsing to/from the UI takes place
- I think this also goes for TimeHelpers. I see this as a higher level
abstraction (the Helpers) on lower level code (ConversionRules), not
the other way around
- There can be still be specific date formats (ie internetDate) that is
being used for API level formatting.

/Jeppe

[Naftoli Gugenheim]

Wait--are you saying TimeHelpers should use ConversionRules and ConversionRules should *not* reference TimeHelpers?
If so it would involve a lot of refactoring, although I think I hear where you're coming from.
I'll try to push the wip-nafg-date branch a bit later. What time zone are you in?

-------------------------------------
Jeppe Nejsum Madsen<je...@ingolfs.dk> wrote:

[Jeppe Nejsum Madsen]

Naftoli Gugenheim <nafto...@gmail.com> writes:

> Wait--are you saying TimeHelpers should use ConversionRules and
> ConversionRules should *not* reference TimeHelpers?

I think so (without knowing too much about the code :-)

> If so it would involve a lot of refactoring,

In that case, suggest we postpone that bit :-)

> although I think I hear where you're coming from. I'll try to push
> the wip-nafg-date branch a bit later. What time zone are you in?

CET

/Jeppe

[Naftoli Gugenheim]

It would be really great if you could take a look at TimeHelpers and my diff (vs. original), on RB and wip-nafg-date.
I asked about your time zone only because I was wondering if you'd be around when I pushed it, but it's pushed.

-------------------------------------
Jeppe Nejsum Madsen<je...@ingolfs.dk> wrote:

Naftoli Gugenheim <nafto...@gmail.com> writes:

CET

/Jeppe

--

[Jeppe Nejsum Madsen]

Naftoli Gugenheim <nafto...@gmail.com> writes:

> It would be really great if you could take a look at TimeHelpers and my diff (vs. original), on RB and wip-nafg-date.

I'll be glad to take a look, but not really sure what it is you want me
to look at :-)

> I asked about your time zone only because I was wondering if you'd be around when I pushed it, but it's pushed.

Ok, it's getting late here but I'll look tomorrow...

/Jeppe

[Naftoli Gugenheim]

At what would make sense to move around, i.e., what I should to move #257 and #258 forward.
Thanks!

-------------------------------------
Jeppe Nejsum Madsen<je...@ingolfs.dk> wrote:

Naftoli Gugenheim <nafto...@gmail.com> writes:

/Jeppe

--

[Jeppe Nejsum Madsen]

Naftoli Gugenheim <nafto...@gmail.com> writes:

> At what would make sense to move around, i.e., what I should to move #257 and #258 forward.
> Thanks!

I think those look good. In the spirit of incremental progress, I think
any other changes should be on new tickets.

/Jeppe

[Naftoli Gugenheim]

So are you saying to leave buildSetFromString pointing to Helpers.toDate for the time being?

-------------------------------------
Jeppe Nejsum Madsen<je...@ingolfs.dk> wrote:

Naftoli Gugenheim <nafto...@gmail.com> writes:

/Jeppe

--

[Jeppe Nejsum Madsen]

On Wed, Feb 10, 2010 at 9:02 PM, Naftoli Gugenheim <nafto...@gmail.com> wrote:
> So are you saying to leave buildSetFromString pointing to Helpers.toDate for the time being?

I'm not sure I follow? I can't find buildSetFromString (in master)
anywhere and the setFromAny that covers string seem to call
ConversionRules?

To summarize: I think all code in MappedDate/Time should go through
ConversionRules

/Jeppe

[Naftoli Gugenheim]

Sorry, type: I was referring to this:

  def buildSetActualValue(accessor: Method, v: AnyRef, columnName: String): (T, AnyRef) => Unit =

  (inst, v) => doField(inst, accessor, {case f: MappedDate[_] => f.st(toDate(v))})

  def buildSetLongValue(accessor: Method, columnName: String): (T, Long, Boolean) => Unit =

  (inst, v, isNull) => doField(inst, accessor, {case f: MappedDate[_] => f.st(if (isNull) Empty else Full(new Date(v)))})

  def buildSetStringValue(accessor: Method, columnName: String): (T, String) => Unit =

  (inst, v) => doField(inst, accessor, {case f: MappedDate[_] => f.st(toDate(v))})

  def buildSetDateValue(accessor: Method, columnName: String): (T, Date) => Unit =

  (inst, v) => doField(inst, accessor, {case f: MappedDate[_] => f.st(Full(v))})

  def buildSetBooleanValue(accessor: Method, columnName: String): (T, Boolean, Boolean) => Unit =

  (inst, v, isNull) => doField(inst, accessor, {case f: MappedDate[_] => f.st(Empty)})

2010/2/10 Jeppe Nejsum Madsen <je...@ingolfs.dk>

/Jeppe

[Naftoli Gugenheim]

In Mapped(Date)(Time)

And 'type' was also a typo. :)

[Jeppe Nejsum Madsen]

Ahh ok.

It's getting late and I'm perhaps not thinking straight but I think
TimeHelpers.toDate should go through ConversionRules. This may cause
circular deps etc. in which case I think buildSetStringValue should go
through ConversionRules.

I don't know mapper internals good enough to really see through the
consequences, but from a user perspective, I think all conversions
should (in the end) go through ConversionRules.

/Jeppe

[Naftoli Gugenheim]

- Until now there were two places in Mapped(Date)(Time) where parsing occurred. setFromAny used LiftRules.parseDate, while buildSetStringValue etc. used TimeHelpers.toDate.
- Do we need methods like setFromAny and TimeHelpers.toDate, which take an Any and pattern match on several types of inputs? Isn't that very un-typesafe?
- Why the inconsistency between setFromAny and buildSetStringValue? Should both use ConversionRules? Should both use toDate which should use ConversionRules?

David, it would be great if you could comment on at least these points. Thanks.

[Naftoli Gugenheim]

(FYI: buildSetXXValue methods are called when loading the field from the database in MetaMapper.scala. This question is relevant in the context of a Mapped(Date)(Time) that corresponds to a database column with a char-based type.)

If the toDate calls in buildSetXXValue are to be replaced with the field-customized parser, then the current design of buildSetXXValue would require the field-level parser-customize method to take the form

def parse(s: String): Box[Date]  // no side effect

rather than something like say

def fromString(s: String): Unit // set the value

On the other hand if buildSetXXValue needs to have some more general parsing logic then it would be better to have the second syntax.

Therefore until someone can comment, my current work for Lift is effectively frozen midair.

2010/2/10 Naftoli Gugenheim <nafto...@gmail.com>

[Naftoli Gugenheim]

Pretty please?