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

Fix for Schedular Allows incorrect Time format #3184 #3185

Merged
merged 2 commits into from
Aug 25, 2023
Merged

Fix for Schedular Allows incorrect Time format #3184 #3185

merged 2 commits into from
Aug 25, 2023

Conversation

leigh-pointer
Copy link
Contributor

Modified code to accept correct time types.

Modified code to accept correct time types.
@sbwalker
Copy link
Member

@leigh-pointer the changes to the Utilities.cs methods are breaking changes which we would want to avoid as third party developers may be using these methods. It is preferable to create new overloads to avoid this.

@leigh-pointer
Copy link
Contributor Author

@sbwalker ok will change.

@leigh-pointer
Copy link
Contributor Author

leigh-pointer commented Aug 25, 2023

@sbwalker
This created two new methods.
public static (DateTime? date, string time) UtcAsLocalDateAndTime(DateTime? dateTime, TimeZoneInfo timeZone = null)

When we want to override a method with a different return type, we are entering into a territory where we need to be careful, as C# does not allow return type-based method overloading by itself. We can't simply change the return type of a method while keeping the same name and parameters.

However, we can use a different approach, which involves giving the methods distinct names while maintaining their parameter signatures and distinct behaviors.

@sbwalker sbwalker merged commit 73c4bce into oqtane:dev Aug 25, 2023
1 check passed
@leigh-pointer leigh-pointer deleted the SchedularTimeControls branch September 25, 2023 13:19
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.

2 participants