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.

Sign up for GitHub

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

Jump to bottom

Support for NodaTime #3624

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from
Draft

Support for NodaTime #3624

wants to merge 14 commits into from

Conversation

igor-tkachev
Copy link
Member

No description provided.

@igor-tkachev
Copy link
Member Author

/azp run test-all

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@linq2dbot linq2dbot mentioned this pull request Jun 23, 2022
Closed
@linq2dbot
Copy link

Test baselines changed by this PR. Don't forget to merge/close baselines PR after this pr merged/closed.

@jogibear9988
Copy link
Member

i suggest a optional assembly parameter.
Cause for example we load assemblys in a different way in our application.
And only load assembly if it is null.

@igor-tkachev
Copy link
Member Author

/azp run test-all

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@igor-tkachev igor-tkachev added this to the 4.1.1 milestone Jun 26, 2022
@igor-tkachev igor-tkachev self-assigned this Jun 26, 2022
@igor-tkachev
Copy link
Member Author

i suggest a optional assembly parameter. Cause for example we load assemblys in a different way in our application. And only load assembly if it is null.

There is an overload with Type parameter. I think it is enough.

@MaceWindu MaceWindu modified the milestones: 4.1.1, 4.? Jul 1, 2022
@igor-tkachev
Copy link
Member Author

/azp run test-all

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@igor-tkachev
Copy link
Member Author

/azp run test-all

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MaceWindu
Copy link
Contributor

update to address v5 changes?

@MaceWindu
Merge branch 'master' into feature/support_noda_time
3f2a1e0 
# Conflicts:
#	Source/LinqToDB/SqlProvider/BasicSqlOptimizer.cs
#	Tests/Linq/UserTests/Issue2560Tests.cs
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jods4
Copy link
Contributor

jods4 commented Jan 27, 2024

Supporting NodaTime without adding a dependency results in quite ugly code.
Wouldn't it be nicer to have a package LinqToDb.NodaTime that adds such support in a straightforward fashion (with a direct dependency on NodaTime)?

The blocker here is the ability to easily define mappings for methods that you can't attribute from outside linq2db core.
There's a similar discussion concerning Regex right now and I made an api proposal: #4392 (comment)

@jods4 jods4 mentioned this pull request Jan 27, 2024
Draft
@linq2dbot linq2dbot mentioned this pull request Jan 27, 2024
Closed
@linq2dbot
Copy link

Test baselines changed by this PR. Don't forget to merge/close baselines PR after this pr merged/closed.

@MaceWindu
Copy link
Contributor

/azp run test-all

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MaceWindu
Copy link
Contributor

/azp run test-all

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MaceWindu
Copy link
Contributor

@igor-tkachev , @sdanyliv regarding @jods4 comment - do you remember what drived you to create this feature?

@MaceWindu
Copy link
Contributor

/azp run test-all

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@linq2dbot linq2dbot mentioned this pull request Jan 28, 2024
Closed
@linq2dbot
Copy link

Test baselines changed by this PR. Don't forget to merge/close baselines PR after this pr merged/closed.

@viceroypenguin
Copy link
Contributor

I agree with @jods4 - I'm not sure I like this being handled implicitly in the core library. If it's possible for this to be distributed as a related package, that would make sense. Maybe this can wait for #4401, in order to be done as a parallel package with the main l2db?

@MaceWindu
Copy link
Contributor

That's not in core library but in linq2db.Tools

@jods4
Copy link
Contributor

jods4 commented Jan 28, 2024

@MaceWindu missed that!
I still think it might not be the most maintainable approach (as Tools are an all-or-nothing deal) but at least it's separated from core.

@igor-tkachev
Copy link
Member Author

We do not need to add any dependencies to linq2db project. If you do not like it, we can move it to linq2db.Tools, add dependency and move it to Examples, or delete it at all.

@jogibear9988
Copy link
Member

If we add mapping for all properties of a nodadatetime object like Hour, Minute, ... as we do for DateTime and DateTimeOffset i think it is usefull to have such a extension (maybe in a Linq2Db.NodaDateTime assembly). But for this we need a extension like in #4401
So someone could use a NodaDateTime Object (don't know wich ones there exist) everywhere in a Linq query same as a DateTime could be used.

viceroypenguin
viceroypenguin requested changes Jan 28, 2024
View reviewed changes
Copy link
Contributor

@viceroypenguin viceroypenguin left a comment

Choose a reason for hiding this comment

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

I'm good with being in linq2db.Tools, though I think a dedicated linq2db.NodaTime would be better, because it could have a direct dependency on NodaTime, which would simplify the mapping code from having to build custom expressions.

Also, it looks like the current code only maps the LocalDateTime type. A proper nodatime handler should map OffsetDateTime as well, as well as possibly LocalDate and LocalTime.

@igor-tkachev
Copy link
Member Author

@viceroypenguin, direct dependency is one of forms of dll hell, something we struggled with unsuccessfully in bltoolkit (previous version of linq2db).

@viceroypenguin
Copy link
Contributor

@viceroypenguin, direct dependency is one of forms of dll hell, something we struggled with unsuccessfully in bltoolkit (previous version of linq2db).

Yes, I agree, at least in the main library.

However, a) doing it in a dedicated library for that support is usually less concerning, and b) dependency management is significantly better today than it was 10 years ago.

@MaceWindu MaceWindu marked this pull request as draft January 29, 2024 09:43
@MaceWindu MaceWindu modified the milestones: 5.4.0, In-progress Jan 29, 2024
@MaceWindu
Copy link
Contributor

Remaining tasks:

  • other NodaType types support
  • (optional) members mapping
  • extension to option instead of mapping schema

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viceroypenguin viceroypenguin viceroypenguin requested changes

@MaceWindu MaceWindu MaceWindu approved these changes

@sdanyliv sdanyliv Awaiting requested review from sdanyliv sdanyliv is a code owner

@jods4 jods4 Awaiting requested review from jods4 jods4 is a code owner

@Shane32 Shane32 Awaiting requested review from Shane32 Shane32 is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@igor-tkachev igor-tkachev

Labels
None yet
Milestone
In-progress
Development

Successfully merging this pull request may close these issues.
6 participants
@igor-tkachev @linq2dbot @jogibear9988 @MaceWindu @jods4 @viceroypenguin