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

Construction from scalar arguments vs. aggregate objects #41

Closed
littledan opened this issue Jul 26, 2017 · 17 comments
Closed

Construction from scalar arguments vs. aggregate objects #41

littledan opened this issue Jul 26, 2017 · 17 comments
Labels
behavior Relating to behavior defined in the proposal documentation Additions to documentation help wanted question

Comments

@littledan
Copy link
Member

In this proposal, constructors take positional parameters, but methods like add take an options bag with named parameters. I was wondering why this option was chosen. Why do constructors not also take such an options bag?

@mattjohnsonpint
Copy link
Collaborator

Because then it would be possible to write something like:

let t = new CivilTime({ minute:30 });  // which hour?
let d = new CivilDate({ day: 31 });    // which year? which month?

The only way to deal with this would be to assume some default values - which IMHO wouldn't be obvious or intuitive.

@mattjohnsonpint
Copy link
Collaborator

As to the options bag for the math functions, #23 has the discussion where we agreed on that approach. Thanks.

@littledan
Copy link
Member Author

@mj1856 Well, what about throwing an exception for cases like that?

@mattjohnsonpint
Copy link
Collaborator

I suppose that would be possible, though I'm not sure it is necessary. There is case to be made for user familiarity with the Date object constructor. Ours would be similar, except for the months field switching from being 0-based to 1-based. Would that be reason enough to switch?

Each approach has its pros and cons. I'm curious what others think. Do you prefer:

var dt = new CivilDateTime(2017, 12, 31, 23, 59, 59, 999, 999999);

or

var dt = new CivilDateTime({
    year: 2017,
    month: 12,
    day: 31,
    hour: 23,
    minute: 59,
    second: 59,
    millisecond: 999,
    nanosecond: 999999
});

@littledan
Copy link
Member Author

(I raised the issue out of an aesthetic preference for the second one, though I'm happy to defer to others here)

@jamiewinder
Copy link

I certainly prefer the normal arguments rather than an options bag. The example above shows how verbose this can be. You'll also always be omitting from one units and lesser if you see what I mean; there is never a situation where someone would want to specify hours and seconds but not minutes, and if you did I'd say it's bad practice to do so.

@domenic
Copy link
Member

domenic commented Jul 27, 2017

I think normal arguments make more sense when the arguments are not optional.

@ljharb
Copy link
Member

ljharb commented Jul 27, 2017

+1 - options objects are great for non-required args, but for required args, there's not really much of a benefit in using options objects imo.

@maggiepint
Copy link
Member

I tend to agree that options doesn't make sense when fields are required.

@icambron
Copy link

icambron commented Jul 27, 2017

I tend to agree with positional here, but a small counterpoint is that it's a bit worse for programmatic use. If you're like me, you end up doing stuff like this a lot:

// This is the most naturally way to keep this data around
// or of pulling off a wire
var someTimeData = {
    year: 2017,
    month: 12,
    day: 31,
    hour: 23,
    minute: 59,
    second: 59,
    millisecond: 999,
    nanosecond: 999999
};

// grr, ok, at least I have ES6, imagine this without it
{year, month, day, hour, minute, second, millisecond, nanosecond} = someTimeData;

// now I can do the thing
new CivilDateTime(year, month, day, hour, minute, second, millisecond, nanosecond);

// So it more naturally takes an array, which seems definitely worse
someTimeData = [
   2017,
    12,
    31,
    23,
    59,
    59,
    999,
    999999
];

// And then you figure out how to apply() but with a constructor, 
// which is something weird like
new (Function.prototype.bind.apply(CivilDateTime, someTimeData));

Anyway, I don't think that trumps how positional parameters naturally enforce requiredness, but it's a thought.

@Yogu
Copy link

Yogu commented Jul 30, 2017

@icambron Would you really store/transfer instants as an object like this? I'd use an ISO-8601 serialization.

@hollowdoor
Copy link

A static .from() operator might be a good compromise.

const likeADate = {
    year: 2017,
    month: 12,
    day: 31,
    hour: 23,
    minute: 59,
    second: 59,
    millisecond: 999,
    nanosecond: 999999
};

const dt = CivilDateTime.from(likeADate);

Something to think about in the future. Similar to Promise.resolve(), or Array.from().

Would you really store/transfer instants as an object like this?

I wouldn't personally, but there could be situations where an object might have some parts of a time.

class Something {
    constructor(){
         this.year = 2017;
         this.month = 11;
    }
    get dateTime(){
        return CivilDateTime.from(this);
        //return new CivilDateTime(this);
    }
}

@gibson042 gibson042 added behavior Relating to behavior defined in the proposal documentation Additions to documentation help wanted question labels Sep 11, 2019
@gibson042
Copy link
Collaborator

I like that the constructors define scalar parameters of decreasing resolution, but we could consider (re)introducing static fromObject methods that define a single parameter and read fields from the corresponding argument, giving equal treatment to Temporal type instances and plain objects.

@gibson042 gibson042 changed the title Named vs position parameters Construction from scalar arguments vs. aggregate objects Sep 11, 2019
@littledan
Copy link
Member Author

Seems like we've settled on positional arguments. Do we want to add this fromObject method (which sounds like a good idea to me) or not?

@obedm503
Copy link

fromObject seem redundant. just from would be better.

@felixfbecker
Copy link

felixfbecker commented Oct 27, 2019

Imo any function taking more than a certain amount of parameters of the same type becomes hard to read and error-prone if it takes those as positional parameters instead of an options bag. Especially in the case of Duration, it makes it unneccessarily hard to create durations of milliseconds, seconds, minutes, hours, days etc because you always need to specify everything starting with years, and what semantic the last passed parameter has is hard to infer when reading the code:

new Temporal.Duration(0,0,0,0,10) // how long is this?
// vs
new Temporal.Duration({ minutes: 10 })

And this is only minutes, now imagine this when working with mili- or nanoseconds. See also @mj1856's example.

Imo this is something that is bad about the current Date API (old and Java-like, not JS-idiomatic). The new Intl, DateTimeFormat and Temporal methods APIs are all way more ergonomic to use and read. Please consider using options objects for the constructors too.

@pipobscure
Copy link
Collaborator

This is obsolete. Conclusion:

  • constructors has positional arguments only
  • static .from methods take property bags to create objects

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior Relating to behavior defined in the proposal documentation Additions to documentation help wanted question
Projects
None yet
Development

No branches or pull requests