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

Time conversion to DateConverter #1031

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andersonluisribeiro
Copy link

Allow the user submit date and time to controller like "21/11/2015 13:10:10" or "21/11/2015 13:10"

@Turini
Copy link
Member

Turini commented Dec 4, 2015

thanks for your PR @andersonluisribeiro, that sounds great!

However I'm not sure if we should change this behaviour, since all of our converters have a default format that can be easily specialized as you can see here

http://www.vraptor.org/en/docs/converters/#overriding-core-converters

I'll hold this one waiting for more dev opinions, ok?

@felipeweb
Copy link
Contributor

I think that we shouldn't change the actual behaviour, because we will broke the pattern, if we will merge this change i think that we need to change the others converters

@lucascs
Copy link
Member

lucascs commented Dec 4, 2015

I think instead of supporting several patterns at once, we could have a config that sets it, and if the user needs more than one pattern, he overrides the default converter. WDYT?

@andersonluisribeiro
Copy link
Author

AS this converter is a date converter, I think it should convert a date with time also when necessary. What do you think?

@Turini
Copy link
Member

Turini commented Jan 30, 2016

I'd keep it as simple as possible, leaving configs extensible just like we're doing now.
If an user wants to support time or any different config, it can be overwritten. Thoughts?

@marcosalles
Copy link
Contributor

IMHO having a default conversion format and the ability to override using @Specializes works fine and dandy.

You might want to think about creating a brand new DateTimeConverter that supports Time adding a new feature instead of changing how the current one works. How about that?

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

Successfully merging this pull request may close these issues.

5 participants