Introducing "Time"

Given the complexity of the domain, I disagree with the assertion that "throws" is good enough. In my experience, it's extremely common to see developers blindly force unwrapping or try!-ing just so they can get the thing they think they want.

I believe that having unsafe as part of the name will help signal that maybe they should be considering whether they truly want to use this or not.

3 Likes

I doubt that devs who'd just shove try! will really care about Unsafe in the name. Especially since try is just as ceremonious as unsafe. Maybe even more since autocomplete won't put in try for you.

8 Likes

In a codebase that sometimes has to deal with the memory unsafe constructs in the Standard library (withUnsafePointer, withUnsafeBytes, etc), being able to e.g. search the codebase for the word ”unsafe”, or to flag it during code review can be really important. Using code that has another definition of unsafe in that environment is simply a non-starter.

So from my perspective, any name that doesn’t include the word ”unsafe” is superior.

10 Likes

I tend to agree with Lantua, throws is signal enough at the declaration as well as use site. If you really want the name to stand out, I don't have any great suggestions, but "unsafe" is definitely the wrong way to go here.

9 Likes

Just as a point of reference, we had a very similar discussion about the use of “unsafe” in Numerics, for an API that could easily overflow and shouldn’t generally be used without checking for that possibility (or a priori knowledge that it would be finite). We decided to drop “unsafe” from the name in order to follow the precedent set by the standard library.

8 Likes

As a plain data point, I'll list below the public GRDB apis that use the word "unsafe" (GRDB is a Swift SQLite wrapper):

struct Configuration {
    var allowsUnsafeTransactions: Bool
}

protocol DatabaseReader {
    func unsafeRead<T>(_ block: (Database) throws -> T) throws -> T
    func unsafeReentrantRead<T>(_ block: (Database) throws -> T) throws -> T
}


protocol DatabaseWriter: DatabaseReader {
    func unsafeReentrantWrite<T>(_ updates: (Database) throws -> T) rethrows -> T
}

class Statement {
    func unsafeSetArguments(_ arguments: StatementArguments)
}

enum ValueScheduling {
    case unsafe(startImmediately: Bool)
}

With the only exception of Statement.unsafeSetArguments, an old method which I believe should be renamed setUncheckedArguments, all other methods can create concurrency bugs that may hit later, elsewhere, or not (as all nasty concurrency bugs). They can only be used safely by users who have a good understanding of their risks. When they enter application code, it should be for a good reason, explained in a comment.

The goal to send a big warning to developpers and code reviewers has the highest priority to me, and "unsafe" fits the bill perfectly.

Now, "unchecked" is a good replacement, when it can be used.

3 Likes

I tend to agree with the objections against "unsafe". "Unsafe" is not supposed to mean "has preconditions which are the client's responsibility". In a narrow form, it would be "can break type safety, memory safety, or exhaustivity" (because breaking any one can lead to the others breaking), but if you were to make an argument for a broader form, I would say it would be "may corrupt data if you break the preconditions", which Time's usage is almost the opposite of.

17 Likes

Exactly. I remember a long similar discussion about adding a failable (or as some prefer to call it, "safe") subscript to Array:

let maybeElement = myArray[ifExists: i]
let maybeElement = myArray[lenient: i]
let maybeElement = myArray[safe: i]    // As if the standard subscript
let maybeElement = myArray[checked: i] // wasn't already checked & safe …
...

Introducing “Time”

Ahhhh finally a way to move through space! :grin:

Looking forward to playing with it! One small critique (which may be personal preference): have some basic usage examples in the initial readme. I'm not a fan of clicking through multiple levels of links just to see what a library is :slight_smile:

4 Likes

I've opened an issue on the repo to continue bike shedding about Unsafe there: Usage of "Unsafe" · Issue #20 · davedelong/time · GitHub

Yep, definitely. I got similar feedback earlier today and will be making this change.

6 Likes

Combine.Scheduler.now use property, not method. :thinking:

1 Like

:-/ Guess I missed reviewing that one!

First: Thank you for providing this. Very well documented, limited in scope. Great library!


Regarding the naming: As you write in the documentation: If you can point to it on a calendar or clock, it's a Value. Maybe call it … PointInTime?

Alternative: Moments. Moments may be recurring as the short Moments<Second, Day> or the longer Moments<Week, Year>. A single incarnation (AbsoluteValue) is:

typealias Moment<Smallest>: Moments<Smallest, Era>

let timeOfMyLife = Moment<Summer>(year: 69)

I do see the problem that the only difference is the plural-s but on the other hand, this rarely is a problem in spoken language.


Difference is a bit generic, too. Here it says:

"February 28th, 2020" represents "the range of all the possible instants between the first instant of this day and the first instant of the next day."

Due to Swift.Range, I do not think that Time.Range would be a good option. Maybe something like Timespan would work? I would not use Time.Span here, it is more likely to be used without the module name.

3 Likes

I think we could agree that "now" (as a property) would and should definitely return a different value at each call ;)

+1 for properties with names implying they are changing.

I think changing its value is ok for a property - but now is an adverb, and those don't make proper names for attributes/traits (under the premise that my synonyms are correct ;-) in general (nouns ftw!).
I might ask a clock about its current time, but it does not have a "now".
To give a completely unrelated example: Technically, each floating point number could have a sin property, but it is much more natural to model that as a function.

1 Like

It's maybe not as common, but "now" can be a noun.

"That was then, this is now."
"We'll be here from now until lunchtime."
"Now would be a good time for us to fix this."

It's an adverb when it's modifying an action, like "I will now pull a rabbit from my hat."

6 Likes

Hi Dave,
Thanks for sharing this. I can definitely see in the code that you thought quite a lot on that topic !
I had one thought while reading some parts of the code and the exemples: you have added static methods on Difference so that the user can write
someDate + .days(2)
Do you think it would be worth adding similar computed properties, so that the user could write
someDate + 2.days

On the pros, it makes the code quite readable imho.
On the cons, it would add a non negligible number of methods to a very widespread type, some could consider that as polution, notably when it comes to autocompletion.

Hi @Jerome, thanks for the thought. I considered adding computed properties like that, but eventually decided that the level of required English comprehension was too high.

1 Like

@hisekaldma @Lantua @lancep @Nevin @jrose @Jens

FYI, the usage of Unsafe in the API has been replaced with Strict on the master branch. :smiley:

Thanks for all your thoughts on the matter!

14 Likes

I'm personally fine with .now() as a method, in part because Dispatch does it. It does return a different value at virtually every call, another semantic point. This makes sense for value types, whereas static properties seem to most commonly return the same value, at least on class types. (UserDefaults.standard or FileManager.default for example, would be expected to return the same instance each time). So to me, it makes sense right off that something like clock.now() never returns the same value twice.