-
-
Notifications
You must be signed in to change notification settings - Fork 416
Added "secs" to core.time as an alternative to "seconds" #173
Conversation
… help with template constraints so if we wanted to add more aliases we could.
In general, I object to layering on aliases that are nothing but alternate spellings. It:
Enhancements to the library should be meat, not trivia. |
This is not exactly the same thing. The argument against "seconds" is that every other term that includes "seconds" is abbreviated to "secs", msecs, usecs, hnsecs, nsecs. But then if you are only interested in seconds units and greater, everything else is spelled out: weeks, days, hours, minutes. "seconds" belongs to this group, while "secs" belongs to the first group. The fact that we are using strings makes this easy to do, because there is no predefined limits to strings. To respond to your points:
This is not super-substantial meat, but it's not trivia. Real programmers have said they have real problems with the abbreviation of seconds. It was an easy change, and cost me next to nothing. If it's rejected, so be it, but at least I tried to help some people. |
IMHO it's a good idea to fix that outlier, |
Using secs instead of seconds would be inconsistent with the rest of core.time and std.datetime. So, I'm completely against replacing seconds with secs. It would needlessly break code. And I'm reticent to add "secs" in addition to "seconds," because it creates a second way to do the same thing. We really should be avoiding needless aliases. Now, this is a very isolated thing in that it's a string passed as a template argument. So, it's not as bad as an additional alias would typically be. And it does avoid the complaints about the fact that it's "seconds" rather than "secs." But on the other hand, it will lead to a bit of confusion, since now there are two ways to do the same thing. So, I'm not completely against it, but I'm not exactly in favor either.
It's because it didn't really make sense to abbreviate years -> seconds IMHO. There are just too many ways to abbreviate them, and they tend to be ugly abbreviations. Using the full words made way more sense. But when you get to milliseconds, it gets ludicrously long not to abbreviate them. So, milliseconds -> hecto-nanoseconds were abbreviated. The usage of seconds is completely consistent in that it's always seconds - never secs - and it's specifically the sub-second units which get abbreviated. And the use of of the sub-second units is fairly restricted in comparison to the other units, since far fewer functions and types support them. Treating them differently makes sense. Now, I do understand that some people see all of the other units with seconds in their name being abbreviated and don't like the fact that seconds isn't, but it's arguably arbitrary either way, and the current usage is consistent. So, I don't know. I'm divided on whether this should be merged in or not. The majority of the vocal people in the newsgroup are for it though. Of course, they're also for adding aliases to dur!"seconds", dur!"minutes" etc. which are seconds(), minutes(), etc., which is definitely adding pointless aliases, which we generally avoid. There seems to be a definite tension between those people who want to have separate functions for stuff rather than using templates and those who want things to be highly generic. A decision one way irritates a bunch of people, and a decision the other way irritates a different set of people. But picking symbol names seems to irritate just about everybody. |
A possibility is to raise a descriptive compile-time error when just "secs" is encountered. That way, dur!"seconds" is still the only valid choice, but people won't be confused when trying to use "secs", as they'll be immediately informed of the proper symbol. There seems to be a lot of really good refactoring here anyway, would be a shame to let that go to waste. |
Actually, such a compile time error wouldn't work very well. Template constraints don't provide a way to give custom error messages. And actually, as nice as the refactoring is, it will make the error messages worse, because instead of listing all of the possible strings, it'll show the eponymous template which failed (e.g. |
*/ | ||
template isScalarTimeUnit(string s) | ||
{ | ||
enum isScalarTimeUnit = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why scalar? Because you couldn't come up with a better name? It's not like the other time units are any less scalar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I misunderstood the term. What would you suggest? I didn't like isNonCalendarTimeUnit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, reading wikipedia definition, scalar isn't what I meant. I meant "this unit can be scaled to other units" So maybe isScalableTimeUnit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still isn't a great name, but I can't think of a better one, and it's definitely better than isScalarTimeUnits
.
I just realized that one of the first requests was that StopWatch.peek.secs worked, which is not something I did. I added a simple alias to it. This may further drive the nail in the coffin of this pull request, but I feel it must be consistent. |
I'm completely against adding any aliases for For instance, The reality of the matter is that we can't please everyone here. Regardless of what names we use, some people are going to complain. And adding additional aliases just increases confusion and the level of inconsistency in code. General Phobos policy is to not add such aliases. Almost all of the aliases in druntime and Phobos are for something which is being deprecated, and the alias will be going away. I'm not convinced that adding So, I'm increasingly against this idea. I understand why you want to do it, but it goes against how we normally operate. It's basically saying that since a group of people don't like a particular name, we're going to just add aliases until one that they like is there, so they won't complain. But then you have to worry about knowing multiple names for the same function, since you'll run into both in code, and it arguably makes learning the library harder. That's the sort of thing that I usually hear being complained about for PHP. Ultimately, people just have to learn to use the library - whatever that library may be - and it's not like it's all that hard to use So, changing the names is just going to break code and not really buy us anything. Adding aliases, on the other hand, causes other problems and is against Phobos policy. It's shot down pretty much every time that it's suggestied. So, I really think that we should just leave this as-is. People will still have to learn what the correct names are - just like they have to do with any module or library that they use. And it's not like the names are bad. They're just not perfect (and neither are the other suggestions). |
Ok |
The current time units for core.time (and std.datetime) are
years, months, weeks, days, hours, minutes, seconds,
msecs, usecs, hnsecs, nsecs
Note how the subsecond values follow a specific abbreviation for "seconds" however, the seconds unit is not abbreviated.
Since it costs pretty much nothing to add "secs" as an alternative to "seconds", I added it.
Most usages of "seconds" was in template constraints, where all the valid units were listed. To make this more DRY, I encapsulated that into some new template-constraint enums such as isScalarTimeUnit(string), used like:
assert(isScalarTimeUnit!"secs" == true);
Also updated some unit tests to test "secs" as well as "seconds". Also going to create related phobos pull request.