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

Jodatime support #80

Open
ismaelgcosta opened this issue Nov 18, 2015 · 10 comments
Open

Jodatime support #80

ismaelgcosta opened this issue Nov 18, 2015 · 10 comments

Comments

@ismaelgcosta
Copy link
Contributor

I need a custom rule for JodaTime fields like Datetime, LocalDate, etc. Can you add this function? Or can I add this function on the project? If are not possible to do in that project, you can create a fixture-factory-extensions, with support for external Java libraries.

Thank you.

@nykolaslima
Copy link
Member

Since not everyone is using JodaTime, maybe it would be nice to think in a fixture-factory-extensions.
But it would be easier to add this support in the core and make JodaTime dependency optional.
@aparra @ahirata what do you guys think about it?

@maurcarvalho
Copy link
Contributor

Hello guys, I watch this project for a while and actually I used it in a
production environment. Maybe it's my opportunity to contribute, follow my
2 cents.
Once Java 8 (and next versions) is using JodaTime as a default date/time
library sounds like a good ideia to support it by default on
Fixture-Factory project.
Like I've said before I can help with this branch in order to make it
happens.

2015-11-18 15:56 GMT-02:00 Nykolas Laurentino de Lima <
[email protected]>:

Since not everyone is using JodaTime, maybe it would be nice to think in a
fixture-factory-extensions.
But it would be easier to add this support in the core and make JodaTime
dependency optional.
@aparra https://github.com/aparra @ahirata https://github.com/ahirata
what do you guys think about it?


Reply to this email directly or view it on GitHub
#80 (comment)
.

@ismaelgcosta
Copy link
Contributor Author

Like a fixture-factory-jdk8 version? I would like to see this (and to contribute also)

@nykolaslima
Copy link
Member

@mauriciojr It would be great. I'm using f-f with JDK 8 and I don't have any problems yet. But support the new date api would be awesome.

Please fell free to go ahead and implement it, this way we can launch a new version supporting java8's date api 👍

@ismaelgcosta
Copy link
Contributor Author

Hi, I forked the project, end started to add support java8's date.

It is still raw, but I would like to hear from you before proceeding. The names may sound a little confusing because the new date has instant class (such as the current method to date).

My solution is not so good I think, if you have suggentions, I would appreciate to listen.

Thanks.

https://github.com/mundodojava/fixture-factory

@ahirata
Copy link
Member

ahirata commented Nov 19, 2015

@mundodojava hey man, maybe I'm missing something but, as far as I can tell, you don't need to create all those functions.

Isn't it just a matter of converting the Calendar to the java.time types? If so, you could just change the CalendarTransformer or create a new Transformer pretty much like the DateTimeTransformer you created.

Also, can you make sure you commit only the lines you have actually changed? Your commits are changing the whole file which makes a bit hard to review and keep track of the modifications.

@aparra
Copy link
Member

aparra commented Nov 19, 2015

Hey @mundodojava,

There is a corner case in convert Calendar to LocalDate using this approach:
LocalDate.of(cal.get(Calendar.YEAR), cal.get(Calendar.MONTH), cal.get(Calendar.DAY_OF_MONTH))

Month in Calendar is zero-based, so if convert today LocalDate to today Calendar you will receive one month ago

Instead of plus one in the month, you could use it: calendar.toInstant().atZone(ZoneId.systemDefault()).toLocalDate()

Please, check your test. The Calendar.getInstance() method returns now but you are changing the date for the past. And finally, check the instructions from @ahirata. :)

@ismaelgcosta
Copy link
Contributor Author

Ok, I will do this changes. As soon as possible, and thank you for the instructions

@ismaelgcosta
Copy link
Contributor Author

I create a new pull request here.

#81

@maurcarvalho
Copy link
Contributor

@douglasrodrigo could you please close this issue too?
Related Pull Request -> #81

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

No branches or pull requests

5 participants