OFFI-132: Shift time #418
OFFI-132: Shift time #418
Conversation
| /// Sets the time shift to a specific value. If both <paramref name="days"/> and <paramref name="seconds"/> are | ||
| /// provided, then the <see cref="TimeSpan"/> values are added together. | ||
| /// </summary> | ||
| public static Task SetShiftTimeAsync(this UITestContext context, double days = 0, double seconds = 0) => |
There was a problem hiding this comment.
Any specific reason why these methods don't just accept TimeSpan, which is the type to handle such values, and the controller to just accept seconds? With double's range, even that would allow setting 8,9864021011838002452723457144492e+67 millennia, so I think we're safe.
There was a problem hiding this comment.
I just passed the action's parameters as-is, without any further abstraction. But I agree, a TimeSpan overload would be cleaner for this extension method.
The controller supports both days and seconds, because I imagine you'd mostly want to shift whole days so that was my first choice. Using the days argument creates an easier to read navigation log as well, ShiftTime/Set?days=30 is easier to understand than ShiftTime/Set?seconds=2592000. But in the off chance that you want to shift less than a day, fractional days are really unwieldy so I added seconds just for completion's sake.
There was a problem hiding this comment.
Is shifting time by whole days, seconds, and both added together so important of a use case to support it with separate overloads? Because this seems really strange to me. TimeSpan has an API for all this. I'd remove this basic arithmetic.
Why I'd use just seconds is to simplify the controller, and use seconds as a serialization of TimeSpan (what it supports with TotalSeconds).
There was a problem hiding this comment.
There was a problem hiding this comment.
The important use-case is to have both options, for the trace to be more human readable as I mentioned in my previous comment. Adding them together is just the simplest implementation I could think of at the moment. In most cases it will be something plus zero, so an arithmetic operation is both easier and more efficient than a conditional operation.
There was a problem hiding this comment.
I understand the controller, then, though it still seems strange to me. How about the extension method overloads, though?
| "Major Code Smell", | ||
| "S6967:ModelState.IsValid should be called in controller actions", | ||
| Justification = "Not relevant in a test-only controller.")] | ||
| public class ShiftTimeController : Controller |
There was a problem hiding this comment.
Any particular reason why almost all of the identifiers are using the action verb "shift time" instead of the operation name "time shift"?
There was a problem hiding this comment.
The reason is that I did it by accident.
There was a problem hiding this comment.
OK then, let's fix this here before releasing this feature or demoing it publicly: #422
OFFI-132