Skip to content

NH-2401 - Method for specifying IType of LINQ parameter #346

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

Closed
wants to merge 3 commits into from
Closed

NH-2401 - Method for specifying IType of LINQ parameter #346

wants to merge 3 commits into from

Conversation

rjperes
Copy link
Member

@rjperes rjperes commented Sep 27, 2014

Implementation and simple unit test
https://nhibernate.jira.com/browse/NH-2401
@oskarb @hazzik: Debugging the code, it is clear that the parameter type is being correctly set, but I'm not sure how to do it in a unit test, have you got any suggestions?

@oskarb
Copy link
Member

oskarb commented Sep 28, 2014

Construct several test cases with basically the same query, but using different "mapped as", in such a way that the outcome of the different queries are different. For instance, if a datetime is treated as a date the time of day will be lost, which could perhaps give different results with a comparison operator. Or a string property with different length.

@rjperes
Copy link
Member Author

rjperes commented Sep 28, 2014

Thanks, will try. BTW, these errors don't occur on my machine...:-/

@rjperes
Copy link
Member Author

rjperes commented Sep 29, 2014

I have added some unit tests and support for type conversions, as currently supported (ToString(), Convert.ToInt32(), Convert.ToDecimal() and Convert.ToDouble()).
This means that it is possible to do this:

from Orders o where Convert.ToInt32(o.OrderDate.MappedAs(NHibernateUtil.Int32)) ...
from Orders o where o.OrderDate.MappedAs(NHibernateUtil.String).ToString() ...

as well as the previous kind:
from Orders o where o.OrderDate.MappedAs(NHibernateUtil.String) ...

@rjperes
Copy link
Member Author

rjperes commented Oct 6, 2014

Any feedback on this? Can this make it to 4.1?

@hazzik hazzik added this to the 4.1.0 milestone Oct 9, 2014
@hazzik
Copy link
Member

hazzik commented Oct 9, 2014

Set to 4.1.0, but I don't like the implementation. Will look closer at the weekends.

@rjperes
Copy link
Member Author

rjperes commented Oct 10, 2014

Why don't you like it?

@@ -10,6 +10,12 @@ namespace NHibernate.Linq
{
public static class LinqExtensionMethods
{
public static T MappedAs<T>(this T parameter, NHibernate.Type.IType type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This random thought: can we have different MappedAs for all IType which can make sense, instead of generic one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this:

public static decimal MappedAs(this decimal parameter, NHibernate.Type.IType type)

?
Sure... it may make sense... want me to update the pull request?
BTW, not related, but why don't we also support, at least, Convert.ToDateTime() in a LINQ query?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hooray, you've learnt how to use line comments... sorry, just trying to joke.

Like this:
public static decimal MappedAs(this decimal parameter, NHibernate.Type.IType type)

Probably

public static decimal MappedAs(this decimal parameter, NHibernate.Type.DecimalType type)

But I see now, that this is a bad idea.

but why don't we also support, at least, Convert.ToDateTime() in a LINQ query?

Just because (c) my 4yo. Just because we forgot to support it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a specific type, we need to allow passing arbitrary types! BTW, why is that a bad idea?

@hazzik
Copy link
Member

hazzik commented Oct 10, 2014

I dont know why I did not like it when I'd written, it was just my feeling.

@rjperes
Copy link
Member Author

rjperes commented Oct 11, 2014

  1. OK for "as" instead of "is"
  2. I'm not checking the method because, if the code gets there, then it's the right one
  3. By traces of debugging you mean having unnecessary variables? Yes, perhaps
  4. You would like to add all the logic to MappedAsGenerator? I think yes, but then we have to check if the arguments are conversions and apply the same logic as I placed in ConvertToXXXGenerator

@oskarb
Copy link
Member

oskarb commented Oct 12, 2014

The generic version will show up in intellisense everywhere. The impact will be mitigated if you gather nhibernate linq queries in e.g. repository or query classes so you don't include the namespace everywhere. But I guess there's not many alternatives.

@hazzik
Copy link
Member

hazzik commented Oct 12, 2014

@rjperes but why in your version .MappedAs applied to a property of a mapped class instead of an parameter?

@rjperes
Copy link
Member Author

rjperes commented Oct 12, 2014

Hmm, now that you mention it... Not sure! Will check it out.

@hazzik
Copy link
Member

hazzik commented Nov 19, 2014

@rjperes any updates?

@rjperes
Copy link
Member Author

rjperes commented Nov 19, 2014

Nopes, maybe in the weekend.

@rjperes
Copy link
Member Author

rjperes commented Dec 1, 2014

Updated pull request to make it clear that the MappedAs extension can be applied to the parameter or the property.

@hazzik hazzik changed the title NH-2401 NH-2401 - Method for specifying IType of LINQ parameter Dec 2, 2014
rjperes added 3 commits April 14, 2015 16:07
Implementation and simple unit test
…vert.ToDouble(), Convert.ToDecimal()) and added unit tests
@hazzik
Copy link
Member

hazzik commented Apr 14, 2015

@rjperes I'm going to remove MappedAsGenerator and other stuff. Leave only changes to ExpressionParameterVisitor, LinqExtensionMethods, and tests.

See here: master...hazzik:NH-2401

@hazzik
Copy link
Member

hazzik commented Apr 14, 2015

It seems that MappedAs from this pull request does not work on parameters at all, because it is being stripped by NhPartialEvaluatingExpressionTreeVisitor before ExpressionParameterVisitor have a chance to process it.

@hazzik hazzik closed this Apr 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants