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

Brevity of API #743

Closed
RobiFerentz opened this issue Jul 9, 2020 · 18 comments
Closed

Brevity of API #743

RobiFerentz opened this issue Jul 9, 2020 · 18 comments
Labels

Comments

@RobiFerentz
Copy link

I'm a lazy programmer, and a crap typist. As such I prefer a bit more brevity in my code. Obviously, these are just my opinions.

  1. Temporal.<whatever> is already long enough, I think avoiding the need for the new keyword would be nice (I know there are those who would prefer not using the new keyword at all in JS, and some make good arguments)
  2. In cases such as Temporal.now.absolute() , if the method is never intended to accept parameters, why not simply make it a read-only property ?
  3. In general I would prefer shorter method names that are still descriptive.

This proposal is a long time coming, if you'll excuse the pun.
Hope it reaches browsers soon!
Thanks for all your hard work!

@ljharb
Copy link
Member

ljharb commented Jul 9, 2020

Temporal.now.absolute() returns a distinct object, that's not === to a previous one, every time you call it. "it takes nonzero arguments" is not a criteria for something to be a function (or to "should" be one).

@RobiFerentz
Copy link
Author

I'm assuming "now" returns a distinct object, is it a method?

Returning distinct objects is not a reason to use functions.

@ljharb
Copy link
Member

ljharb commented Jul 9, 2020

It very much is a reason for that.

Put another way, using an accessor property to return distinct objects every time is exceedingly confusing and strange, so the burden of proof is on that scenario imo.

now is not a method and doesn't "return" anything; Temporal.now is just a namespace object that contains methods.

@pirate
Copy link

pirate commented Jul 9, 2020

High character counts for simple tasks is also my main gripe after skimming through the cookbook, take for example the suggested way of converting from old Date objects to the new API:

const absolute = Temporal.Absolute.fromEpochMilliseconds(legacyDate.getTime());

Thats more than 50 characters of Temporal for what's certainly going to be one of the most common tasks in the initial years of using this API. Intuitively I'd expect it to just be Temporal(legacyDate). I also noticed similar issues with many of the other common tasks in the cookbook.

@ryzokuken
Copy link
Member

Temporal. is already long enough, I think avoiding the need for the new keyword would be nice (I know there are those who would prefer not using the new keyword at all in JS, and some make good arguments)

I think that's a pattern we've consciously moved away from, so it won't be the best idea.

In cases such as Temporal.now.absolute() , if the method is never intended to accept parameters, why not simply make it a read-only property ?

Precisely what @ljharb said.

In general I would prefer shorter method names that are still descriptive.

The champions group (and I personally) share(s) the same values. If you have any specific proposals, I'd love to hear them and consider changing them.

@ryzokuken
Copy link
Member

ryzokuken commented Jul 9, 2020

@pirate if you have specific suggestions for any of these methods, we'd gladly consider them. Besides, I don't specifically agree that character count should be given a large emphasis in API design? Brief APIs are great, but not at the cost of clarity, especially in 2020 when most developers use an editor with code completion functionality.

@pirate
Copy link

pirate commented Jul 9, 2020

@ryzokuken why not allow Temporal(legacyDate)?

@ljharb
Copy link
Member

ljharb commented Jul 9, 2020

Temporal is a namespace object, not a function. new Temporal.Absolute(legacyDate) might make sense tho?

@RobiFerentz
Copy link
Author

RobiFerentz commented Jul 9, 2020

Specifically on the Absolute object, I'd remove the Epoch word, as it should be implied by it being an Absolute datetime.

To shorten it even further, you could always use something like .from(number, 'ms') and other known shorthand time units to specify how the number should be treated. Though I get many see that as a rather ugly option, though it could be an alias for us lazy people. An added bonus is that further time units could be incrementally supported without adding more methods.

@bijection
Copy link

bijection commented Jul 9, 2020

For constructors, how about accepting named args, instead of having many .from* methods?

For Absolute the method signature could look like this:

Temporal.Absolute(source: bigint | Date | {seconds: number} | {milliseconds: number} | {nanoseconds: number})

This would have the added advantage of letting APIs respond with things like {timestamp: {seconds: 239847293}} which could then be passed into temporal for further manipulation.

@ptomato
Copy link
Collaborator

ptomato commented Jul 9, 2020

Thanks for the feedback, by the way! I think there are a couple of takeaways here, the main one being that brevity is appreciated 😄

One thing not mentioned yet so far in the thread is that Temporal doesn't require you to use the types with new. We consider the constructors a bit more 'low-level' than the Temporal.<whatever>.from() static methods, which do take more than one kind of input, and that was intentional for exactly the reason you mentioned in the OP — some would prefer not using the new keyword at all.

There is more discussion on methods vs. properties at #724, you might want to subscribe to that discussion as well.

@rlue
Copy link

rlue commented Jul 10, 2020

Brief APIs are great, but not at the cost of clarity, especially in 2020 when most developers use an editor with code completion functionality.

@ryzokuken In the case of a module as fundamental as Temporal, I disagree. The most important benefit of shorthands (in both natural and constructed languages) is not that they are easier to write, but that—given the expectation of some prior knowledge or familiarity with a subject—they are easier to read.

Consider:

  1. "The square of the radius of a circle (centered at the origin) is equal to the sum of the squares of the x- and y-coordinates of the points it comprises."
  2. r² = (x² + y²)

The first contains far more information, and is thus clearer to people with no prior knowledge. But to those with prior knowledge, it's profoundly burdensome. The latter is not only just as clear, but short enough to be apprehended at a glance (arguably below the level of conscious thought).

The question is whether the audience can be reasonably expected to seek out this knowledge if they don't have it. If not, then the audience must instead devote their finite attentional/cognitive resources to translating a long string of words into an idea they already possess. I would argue that converting a Date to a Temporal is definitely something JS developers can be expected to internalize.

Clarity over convenience may be a best practice in general (i.e., for specialized, obscure, or third-party libraries without wide usage), but applied to core modules like Temporal, they will erode the utility and readability of the language in the long-term.

@ryzokuken
Copy link
Member

@rlue I whole-heartedly agree with your analogy. That why I said that I do sympathize with the need for a more brief API surface. I did however mention that it should not happen at the cost of clarity, which should infact result in a poor reading experience.

@justingrant
Copy link
Collaborator

Many of the comments above deal with ergonomics & brevity for constructing Temporal.Absolute instances (esp. from legacy Date), so I coalesced that feedback into #751. Let's discuss further over there.

I also opened #750 to track whether Temporal.Absolute getters should be properties like abs.epochMilliseconds instead of methods like abs.getEpochMilliseconds().

@justingrant
Copy link
Collaborator

I also opened #750 to track whether Temporal.Absolute getters should be properties like abs.epochMilliseconds instead of methods like abs.getEpochMilliseconds().

This change has landed.

image

Many of the comments above deal with ergonomics & brevity for constructing Temporal.Absolute instances (esp. from legacy Date), so I coalesced that feedback into #751. Let's discuss further over there.

#751 has landed too:

image

In addition, a big brevity improvement added recently is the ability to use strings instead of Temporal objects as arguments to Temporal methods. For example:

image

@ptomato
Copy link
Collaborator

ptomato commented Jan 14, 2021

"More brevity" was by far the most common feedback that we got and I think we've addressed it, even if not in the exact ways suggested here, with the changes Justin mentioned above and others. I hope that it feels less verbose as a whole now! Closing this.

@ptomato ptomato closed this as completed Jan 14, 2021
@stiff
Copy link

stiff commented Jan 21, 2021

Maybe it's worth considering this API as a part of standard library rather than default part of language?

Like in c++, there is no problem to #include , same could be here import now from 'Temporal'.

Personally I very much doubt it would be a pleasant experience to type dozens characters for basic operations. Not only in IDE, but also in REPLs and so.

@ryzokuken
Copy link
Member

@stiff thanks for the suggestion, there have been discussions regarding built-in modules at https://github.com/tc39/proposal-built-in-modules. That said, I feel that built-in modules won't be adopted for quite some time and it's unfair to delay Temporal on it.

That said, note that something like const { now, PlainDate, ZonedDateTime } = Temporal should work just as well as the example you suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants