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

DateBuilder that allows DateOnly building too #2279

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

Corniel
Copy link
Contributor

@Corniel Corniel commented Aug 27, 2023

As illustration of the discussion in #2234

@github-actions
Copy link

github-actions bot commented Aug 27, 2023

Qodana for .NET

4 new problems were found

Inspection name Severity Problems
Convert local variable or field into constant (private accessibility) ◽️ Notice 4

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at qodana-support@jetbrains.com

@ITaluone
Copy link
Contributor

ITaluone commented Sep 7, 2023

@Corniel I like this :)

Although I would suggest to drop the AsOffset() and WithOffset() methods inside the DateBuilder struct, and only use extension methods for this, like:

public static DateTimeOffset WithOffset(this DateTime self, TimeSpan offset) => new(self, offset);
public static DateTimeOffset WithOffset(this DateBuilder self, TimeSpan offset) => new(self, offset);
public static DateTimeOffset AsOffset(this DateTime self) => new(self, 0.Hours());
public static DateTimeOffset AsOffset(this DateBuilder self) => new(self, 0.Hours());

This will address #2234 (comment)

@Corniel
Copy link
Contributor Author

Corniel commented Sep 7, 2023

@Corniel I like this :)

Although I would suggest to drop the AsOffset() and WithOffset() methods inside the DateBuilder struct, and only use extension methods for this, like:

Why define the offset methods as extensions on the date builder? It's only purpose is just to do things like that?!

@IT-VBFK
Copy link
Contributor

IT-VBFK commented Sep 7, 2023

Why define the offset methods as extensions on the date builder?

Because it shouldn't matter if you call AsOffset()/WithOffset() after the date "thing" like 10.May(2023).AsOffset() or after the time "thing" like 10.May(2023).At(...).AsOffset().

Now this is not possible, because AsOffset() is not recognized when chaining after At(...).
And if you introduce AsOffset() as an extension method, the AsOffset()in the DateBuilder becomes useless.

Do you see my point?

@Corniel
Copy link
Contributor Author

Corniel commented Sep 8, 2023

@IT-VBFK: I ment, that AsOffset should not be an extension for the DateBuilder. For DateTime it should.

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

Successfully merging this pull request may close these issues.

None yet

3 participants