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

Better support for dates and strings in LINQ #253

Closed
wants to merge 8 commits into from

Conversation

pieceofsummer
Copy link

While playing around with some custom serializers and complex classes, I've found a few errors and a bunch of not implemented functions, so I spent an evening trying to make the world a better place :)

Hope it will be helpful.

1) Overload for Substring() with single arg now works
2) Overload for Equals() with single arg now works
3) Also added static version of String.Equals()
4) Added some of the overloads for String.Concat()
5) More checks for correct number of arguments
1) Added DateTime.ToString() (via $dateToString)
2) Added support of AddDays(), AddHours(), AddMinutes(), AddSeconds() and AddMilliseconds() operations

AddYears() and AddMonths() are currently not implemented to not overcomplicate requests, while Add() and Subtract() are not implemented because TimeSpan is not convertible to BsonValue
@craiggwilson
Copy link
Contributor

Hi @pieceofsummer,

Thanks so much for the PR. Couple things I noticed.

  1. For code consistency, we always use braces with conditional statements, even one liners.
  2. There aren't any tests. While I'm sure your implementations are perfect, we really need tests to ensure we don't break them in the future.

If you could do both those things, we'll look at pulling this in.
Thanks,
Craig

@pieceofsummer
Copy link
Author

Hi @craiggwilson,

  1. No problem. I like short one-liners, but it's not too hard to sacrifice them for the sake of greater good :)

  2. I highly doubt they're perfect, but I already was too sleepy to think of how to set up test cluster early in the morning :) I'll add some unit tests later.

There's also that recent issue with .NET Core when connecting to DNS endpoints on Unix, which makes even existing tests to fail for me… :(

@pieceofsummer
Copy link
Author

@craiggwilson, I'm also a bit confused with the current implementation of math functions.

Docs say almost all math functions (except $pow and $log, for obvious reasons) take just a single number, but the driver gives them an array (with single number inside). Furthermore, Mongo server accepts both dialects, while being rather strict about other functions' arguments.

Can you please clarify this a little?

@craiggwilson
Copy link
Contributor

Formally, the server wants an array as the argument. For single-valued args, they accept just a value without being in an array. I forgot what we currently do, but might as well follow whatever pattern we are currently using.

@pieceofsummer
Copy link
Author

Added some unit tests, hope it's ok now :)

@craiggwilson
Copy link
Contributor

This is awesome. We'll discuss and see if there is anything left to do. Might ask you to squash and rebase on master just before the pull to ensure you get credit properly. Will let you know.

@pieceofsummer
Copy link
Author

pieceofsummer commented Sep 26, 2016

Should I rebase or close this?

I kinda see $dateToString implemented in 79853e2, do you plan to add other date functions as well?

@elliott-pd
Copy link

I have made an updated version of this PR: #351 which is based off current master.

@jyemin
Copy link
Contributor

jyemin commented Nov 18, 2020

Closing as superseded by #351

@jyemin jyemin closed this Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants