Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

Conversation

jmdavis
Copy link
Member

@jmdavis jmdavis commented Jun 12, 2014

Apparently, some folks found it confusing, because it just so happens
that get!"weeks"() was identical to total!"weeks", since weeks were the
largest units. However, it does seem to highlight how folks have
misunderstood what the individual getters do (which is why they're being
deprecated).

Apparently, some folks found it confusing, because it just so happens
that get!"weeks"() was identical to total!"weeks", since weeks were the
largest units. However, it does seem to highlight how folks have
misunderstood what the individual getters do (which is why they're being
deprecated).
@yebblies
Copy link
Contributor

Are you planning to re-introduce them with the intuitive behavior once the deprecation period is over? Because if you are maybe weeks should just stay as-is.

@MartinNowak
Copy link
Member

Auto-merge toggled on

MartinNowak added a commit that referenced this pull request Jun 12, 2014
Adjust deprecation message on Duration.weeks.
@MartinNowak MartinNowak merged commit 6b165e4 into dlang:master Jun 12, 2014
@jmdavis
Copy link
Member Author

jmdavis commented Jun 12, 2014

Are you planning to re-introduce them with the intuitive behavior once the deprecation period is over? Because if you are maybe weeks should just stay as-is.

No, I don't intend to reintroduce weeks or any of the other individual getters. They're too error-prone. I think that it's just safer to make it so that code has to be more explicit.

@schveiguy
Copy link
Member

I'm not going to push this specific issue any more, but can we please avoid merging things within 1 hour? I think at least a day or even 2 should be given for review. Case in point, this message is not any better than the previous, in fact, I think it is more confusing. Obvious errors or typos can be merged instantly, obviously. Perhaps auto-merge can be modified to wait a specified amount of time since the last change before merging.

@jmdavis
Copy link
Member Author

jmdavis commented Jun 12, 2014

this message is not any better than the previous

I honestly do not understand what is wrong with the message. You thought that the previous one wasn't clear enough because get!"weeks"() happens to be the same as total!"weeks", so I switched it to talk about get - which weeks wraps - instead of weeks specifically. Either way, it's quite specifically telling you to use split instead, and it's telling you that the reason is because the function was being confused with total. What else would you want?

As it stands, it seems like the person who brought up the original message in the newsgroup was confusing it with total and then just couldn't understand the difference, because it just so happens that weeks was the same as total!"weeks", because weeks were the largest units.

@schveiguy
Copy link
Member

I honestly do not understand what is wrong with the message.

I think sometimes less is more. Just saying weeks is deprecated, use total!"weeks" instead would have been sufficient. In any case, it's not important now, I don't think it's worth making another pull request unless the new message proves to be too confusing, but I think we've spent enough time on this :)

@MartinNowak
Copy link
Member

I don't think it's worth making another pull

Why not? Pull requests are for free and if you can improve the message that's a good thing.

@schveiguy
Copy link
Member

Pull requests are for free

Pulling a request invalidates all current tests. But if the new message doesn't confuse people, then it's definitely not worth it. I think we should wait until it proves to be an issue.

@yebblies
Copy link
Contributor

Pulling a request invalidates all current tests.

So? It's not like the tester is the bottleneck.

@schveiguy
Copy link
Member

Hey, if someone wants to make a new pull request to change it, great! I personally do not think it's worth the time at this point.

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

Successfully merging this pull request may close these issues.

4 participants