search

ryanheath / RavenDB-NodaTime

Noda Time support for RavenDB
Other
20 stars 14 forks source link

Add support for DateInterval type #26

Closed tiesmaster closed 3 years ago

tiesmaster commented 4 years ago

First of all, I really like that you created this package. We have it installed, and works like a charm ;)

Well, up to the point where we started to use a couple more exotic types of NodaTime, as mentioned in the title. I would really love to contribute a PR to add support for this. Are you open to this?

tiesmaster commented 4 years ago

@ryanheath Did you see this issue, perhaps? It would be great if we can add support like this in this package.

ryanheath commented 4 years ago

Hi @tiesmaster I just now saw your request. Yes! Submit a PR.

Thanks

// Ryan

tiesmaster commented 4 years ago

No worries, I was swamped the last couple of weeks, so wouldn't have been able to work on this. It looks like I have now, so I might be able to put in a PR soon (but no promises ;)).

ryanheath commented 4 years ago

Which RavenDb version are you using? YearMonth is a NodeTime 3.x feature, it would be nice to support NodaTime 3.x for both v42 and v50. I think it's wise to start from feature/upgrade-v42

tiesmaster commented 4 years ago

Ah, I didn't notice your message until I came here to report about the problem that master is still using an older version of NodaTime. I didn't even knew that YearMonth is added this recently, but this also explains why I didn't find that type initially when developing our budgeting algorithm doing things with months 😅

Which RavenDb version are you using? We're using RavenDB 4.2

I already continued on the master branch, with upgrading to NodaTime 3.x, and to see what the tests tell me (because I was curious). The funny thing is, I've copied from the NodaLocalDateTests (and adopted to YearMonth), and all pass without any changes to the production code. This is really surprising, and I cannot explain this... Do you have any clue? (My work is in this branch)

I'm gonna rebase my branch on top of the feature/upgrade-v42 branch (I'll rename my local branch, so the branch I referenced won't get overwritten), and see what where that leads me.

ryanheath commented 4 years ago

It depends on how the YearMonth is actually stored in the json mark up. I'm not sure if we have tests for this, I vaguely recall there should be some ...

Maybe RavenDb 5 is a lot smarter now. 🚀 Let's see how RavenDB 4.2 handles it.

// Ryan

tiesmaster commented 4 years ago

Indeed, the YearMonth serializes without any problems using Json.net, I was mistaken. We're not persisting our YearMonth property, only the DateInterval.

We have custom mapping for that, but with this new insight, I'm not sure we need custom mapping. Though, I haven't tested it out yet: I was a bit busy the last couple of days, and I will be going on holiday tomorrow for 2 weeks, so I'll back to you then ;)

tiesmaster commented 3 years ago

Hi @ryanheath. I was on holiday for a couple of weeks, so I didn't manage to continue with this, until now. I just checked the serialization for the DateInterval type, and that one does need custom mapping.

When serializing that type to Json, you get a list of all the days in that interval (useless, and doesn't deserialize). I'm going to continue with this, did anything change? Branch-wize, or should I still target the feature/upgrade-v42 branch?

ryanheath commented 3 years ago

Yes, please target v42 first, the changes will probably merge easily into v50.

// Ryan

ryanheath commented 3 years ago

Thanks!