[patch][spec] There is no simple way to send dates, times etc as parameters
Reported by Martin Kihlgren | June 2nd, 2008 @ 04:28 PM | in 0.9.6
In RoR I grew accustomed to being able to send some parameters as some kind of magical composite parameter. Example: Date and Time objects get multiple parameters each, with each parameter defining the year, month, day, hour, minute, second etc of the object.
In merb that doesn't seem possible right now. So I built it!
There are two patches against merb-core, that adds a CompositeParamParser class under Request, and lets Request use it when producing parameters.
The CompositeParamParser go through all params and call predefined methods on predefined classes with all params that belong together according to a param index marker.
The other patch is a patch against merb-plugins that adds a whole lot of new #*_field and #*_control methods to Helpers::Form, that create this kind of magical parameters for simple to use date_control and time_control bliss.
Comments and changes to this ticket
-
-
-
Michael Klishin (antares) June 2nd, 2008 @ 05:37 PM
- → Milestone changed from “” to “1.0”
- → State changed from “new” to “open”
-
Martin Kihlgren June 2nd, 2008 @ 07:26 PM
Yet another version of the second patch ... it seems the call signature needs testing before it becomes perfect :)
-
Martin Kihlgren June 5th, 2008 @ 05:28 AM
Added options to not show all parts of a time, date or date_time field or control.
-
Michael Klishin (antares) June 29th, 2008 @ 09:00 AM
- → Tag changed from “” to “form_helpers merb-core merb-plugins patch spec”
- → Assigned user changed from “” to “Yehuda Katz (wycats)”
Helpers are being rewritten now, but I think this is a great addition for new version.
-
Martin Kihlgren June 30th, 2008 @ 10:44 AM
Couldn't you add this to the current version as well? It is such a pain to have to distribute my own gems with the app :((
-
Michael Klishin (antares) June 30th, 2008 @ 10:48 AM
I will check with people who do the rewrite to see if it would complicate merges for them. That's the only concern I have.
-
-
Michael Klishin (antares) August 2nd, 2008 @ 09:27 PM
Should this be closed, is it fixed in new merb-helpers?
-
Michael Klishin (antares) August 2nd, 2008 @ 10:55 PM
- → Assigned user changed from “Yehuda Katz (wycats)” to “GMFlash”
-
Michael Klishin (antares) August 2nd, 2008 @ 11:05 PM
- → Milestone cleared.
-
-
Michael Klishin (antares) August 14th, 2008 @ 02:07 PM
- → Milestone changed from “” to “0.9.5”
-
Martin Kihlgren August 21st, 2008 @ 12:45 PM
If I may ask, is there any special reason why this is not yet in merb? It is a very very useful feature (at least I personally need it), it is specced out, and it works fine in my project.
Does anyone feel that it is too ugly, or that there is a better solution, or that it is unnecessary?
-
Michael Klishin (antares) August 21st, 2008 @ 12:59 PM
No, we just were completely refactoring form helpers and I felt like it's not a good time to apply this.
If you can rebase you patches after 0.9.5 against HEAD, please do it and attach them here. I'll have a look.
-
Martin Kihlgren August 21st, 2008 @ 03:21 PM
Oh my god, yes you have refactored the helpers alright...
I will attach the rebased patch for merb-core. But I need help with the patches against merb-plugins, since I don't have time to read through and understand how they work right now.
Note that you can apply the patch against merb-core totally independent of the patch against merb-plugins - it just opens up the possibility of adding parsers for composite parameters.
-
Martin Kihlgren August 21st, 2008 @ 03:31 PM
- no changes were found...
-
Martin Kihlgren August 21st, 2008 @ 04:59 PM
Ok, I may have misjudged the difficulty, here is a patch on merb-plugins that does the same thing the old patch did.
There are no specs in this one, I haven't had time to build them, but please look at the patch and comment.
-
Martin Kihlgren August 25th, 2008 @ 01:13 PM
I append an updated version of the form helpers patch, and a patch that adds specs for it.
-
Martin Kihlgren August 25th, 2008 @ 04:03 PM
Here is an improvement that cleans up a lot of non-valid html inadvertently created by the time and date helpers.
Also included is a bug fix for a mindfart someone made :)
-
Martin Kihlgren August 25th, 2008 @ 04:37 PM
Once again all patches against merb-plugins in one patch.
-
GMFlash August 25th, 2008 @ 08:08 PM
This is a very big patch and introduces a lot of changes including:
- modifying the Time class
- modifying Merb::Request
- adding several hundred lines of code to the helpers
I think this should be given some more time for consideration in order to find the best approach to dealing with dates and times rather than emulating the UI widgets from Rails. Just to be clear, the idea is still on the table but all options should be considered before committing.
-
Martin Kihlgren August 25th, 2008 @ 06:37 PM
Good. I just want a discussion to exist, so that I will know how to handle this.
Today I benchmarked and noticed that the time to parse parameters increases about 20-25% with this patch - another reason to consider carefully.
One solution that I would like is to make the query parsing pluggable, so that a simple switch could make the composite param parser do its stuff.
There are other ways of doing it instead of modifying the Time class, but that seemed like the simplest solution.
The mass of lines added to the helper class however, is hard to avoid, imho :/
-
GMFlash August 25th, 2008 @ 08:20 PM
I think that date_select, time_select, and date_time_select all follow similar patterns which can be extracted to reduce duplication of code. For example, what is different about the date_select and time_select aside from the field names? Also in date_time_select it is basically a combination of the date and time helpers so why duplicate all of their code instead of using them directly to build the date+time selects?
Instead of putting the code into a single module it might be better to take the approach used in Rails which is to have separate modules for the basic helpers, select helpers, and date/time helpers.
These are a few approaches I would like explore with the helpers.
-
Martin Kihlgren August 25th, 2008 @ 11:54 PM
Yes, I agree that there are lots of similarities between them, and I welcome any effort to try and reduce the duplication.
The reason I have not worked more in that regard is simply that there are so many field and option and method differences that I suspected the gains would not be worth their cost in extra complexity.
But please improve the solution in a way that you find acceptable!
I will probably soon (not right now, now I have to produce a bit at work again :) look into producing a patch that only enables composite parameters and accompanying performance cost when wanted, in the form of a plugin or similar. Then it would be simple to just let the date and time helpers depend on that plugin, and let merb core be more minimal.
-
Michael Klishin (antares) August 26th, 2008 @ 01:53 AM
- → Milestone changed from “0.9.5” to “0.9.6”
-
Yehuda Katz (wycats) September 2nd, 2008 @ 12:08 PM
- → State changed from “open” to “resolved”
turns out that DM supports converting hashes directly to dates an times. zond is pleased with that solution :)
Please Login or create a free account to add a new comment.
You can update this ticket by sending an email to from your email client. (help)
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
Bryan Ray
Ludo van den Boom