electricessence / TypeScript.NET

A JavaScript-Friendly .NET Based TypeScript Library (Moved)
https://github.com/electricessence/TypeScript.NET-Core
Other
251 stars 36 forks source link

TimeSpan accessor values are not correct compared to .NET Standard spec #65

Closed ciband closed 6 years ago

ciband commented 6 years ago

First, thank you for developing this library.

I am using the TimeSpan class and noticed that its accessors (hours, minutes, seconds) are not returning the correct values. They are returning TotalHours, TotalMinutes, etc... This behavior does not match the .NET spec for TimeSpan.

I would suggest that the constructor be updated to set these members to their proper values and add the Total* members to fill out the interface.

I attempted to start to do this and generate a Pull request but could not figure out how to build/test this project as I am a noobie to TypeScript. Just starting my first .NET Core/Angular 4 project.

Thank you for your consideration.

electricessence commented 6 years ago

I'm glad to have your critique... Straddling the line of .NET/C# and JS/TS... I spend significant 'span of time' :grin: considering the Time classes in this library...

There needed to be a consistent interface that matched how to measure one type of time measurement or another. So some compromises were made. If you notice the interface https://github.com/electricessence/TypeScript.NET/blob/master/source/System/Time/ITimeQuantity.d.ts has a 'total' property that exists for all of its sub classes. With TimeSpan I attempted to model an immutable time structure in order to preserve the 'read-only' nature of it and prevent unexpected weirdness in JavaScript.

TimeSpan by definition should simply be a measure of the amount of time (quantity +/-). In my experience it is simply more common to measure the 'total' time. As a JS/TS developer I found the default way TimeSpan is specified to be confusing. Typically if you want a value from TimeSpan you want to know the 'total' hours, etc... Flooring then is up to you.

In order to gain a better interface that makes more human sense I added .time to TimeSpan which returns a ClockTime object (https://github.com/electricessence/TypeScript.NET/blob/master/source/System/Time/ClockTime.ts). It has the proper segmentation of values if by chance you need a measure of what time of day in addition to how many days.

// Example
function outputTime(ts:TimeSpan)
{
  console.log("Total Minutes: ",ts.minutes);
  console.log("Total Minutes: ",ts.total.minutes);  // same as above
  console.log("Time Hour: ",ts.time.hour); 
  console.log("Time Minute: ",ts.time.minute); 
}

Considering the syntax above, imagine how the formatting string looks in Angular if you want to display the time? {hour}:{minute} Notice that I have yet to implement a formatting provider from .NET as it's a nightmare. :(

This also includes a change in DateTime which has a .timeOfDay property that returns a ClockTime object as well... Which should make sense to the consumer since ClockTime's properties are more line with TimeSpan's spec.

You may also notice that when trying to find the time between (https://github.com/electricessence/TypeScript.NET/blob/master/source/System/Time/DateTime.ts#L382-L388) two dates, it does return a TimeSpan which makes sense.

If changing from using TimeSpan to ClockTime solves your problem please let me know. Otherwise I'm still open to suggestion an appreciate your input.

ciband commented 6 years ago

Thank you for the quick and detailed response.

What ClockTime does is exactly what I expected TimeSpan to do so. My particular use case is C# -> JSON -> TypeScript so the TimeSpan on either end would be nice for my purposes.

As a noobie TS/JS developer and having more roots in C#, I had the exact opposite reaction you did to TimeSpan. The C# way make sense, lol.

If the goal of the project is to map to .NET types then I would think you would want to match the behavior, but that is just my 2 cents.

Thank you for the library and the additional information to keep things going. Please feel free to close the issue.

ciband commented 6 years ago

One other thing i noticed is that for ClockTime the members names are not the same as TimeSpan, e.g. TimeSpan.hours vs. ClockTime.hour. I would recommend matching the plural-ness of everything.

Thanks again.

electricessence commented 6 years ago

As a noobie TS/JS developer and having more roots in C#, I had the exact opposite reaction you did to TimeSpan. The C# way make sense, lol.

Well if you look further into the System.Time namespace (which doesn't exist in .NET) you'll see there's even more classes centered around time. Like TimeUnit and TimeUnitValue which allow for capturing and transforming a specific measure of time and its unit of measure. TimeSpan settled into it's current form because by it's name and nature is a 'quantity' of time. And before you would type .TotalSeconds for example. Now you simply type .total.seconds. :)

electricessence commented 6 years ago

One other thing i noticed is that for ClockTime the members names are not the same as TimeSpan, e.g. TimeSpan.hours vs. ClockTime.hour. I would recommend matching the plural-ness of everything.

So the reasoning for the difference in names: hours vs hour is to ensure, from an interface perspective, that there is no way that you could make the mistake of passing the time of day to a method that expects a quantity of time and vice versa.

Take note that unlike C#, TypeScript has what's called type 'equivalency'. Which is really awesome, but for example if you have two interface signatures that match, they are considered equivalent and TypeScript will treat them as the same.

This is a type/time problem I've encountered before that can be a bit frustrating when one or the other gets confused. Consider the benefit of having two different classes that represent two different types of measure instead of just one. You can then write APIs that can differentiate.

interface ICheckTheTime
{
  whatTimeIsIt(ClockTime time);
  howMuchTimeHasPassed(TimeSpan time);
}
electricessence commented 6 years ago

As thanks for your 'time' 😜:grin: : https://www.udemy.com/stepping-up-to-typescript-fundamentals/?couponCode=SUPERFRIENDS Enjoy my course for free with this link. I suggest claiming it immediately before someone else does. 😏

I started with JavaScript in the late 90s. I was then primarily C# for many years, then back to ActionScript which was basically TypeScript. Then C# again, then finally TypeScript. I would primarily consider myself C#, but at this point the lines are blurred.

The goal of the project is to have a JS friendly library, not necessarily to map exactly to the .NET framework. It has to make sense to a JavaScript/TypeScript developer. If you need exact parity of all things .NET I highly recommend checking out JSIL. 😏 Which now emits TypeScript declarations. 😃 http://www.jsil.org/

electricessence commented 6 years ago

Oh and just a bonus... If you want to do a string format from ClockTime... Check out... https://github.com/electricessence/TypeScript.NET/blob/master/source/System/Text/Utility.ts#L107-L143 System.Text.Utility.supplant(format,...args) or the more familliar System.Text.Utility.format(format,...args).

import {supplant} from "typescript-dotnet-umd/System/Text/Utility";
var timeString = supplant("{hour}:{minute}:{second}",DateTime.now.timeOfDay);

This doesn't work perfectly because of the missing leading zeros :(.... Which I need to find a solution for... Working on a proper .toString(format:string,formatter:IFormatter):string setup.