Turn enforce into an eponymous template + undocument enforceEx by wilzbach · Pull Request #6086 · dlang/phobos

wilzbach · 2018-01-28T17:35:14Z

Follow-up to #5234 and #6080.

tl;dr summary

However, if enforce can't fully replace enforceEx, then it needs to stick around for at least one more release so that we can have a release where enforce can fully replace enforceEx. That way, someone can build their code with both the latest release and master without getting a bunch of deprecation messages. And clearly, from how enforceEx is being used in Phobos, enforce needs to be improved first. So, please create a PR to improve enforce, and then we can deprecate enforceEx after the improved enforce has been put out in a release.

I already added the warning in the documentation and removed enforceEx from the bookmark summary at the top.

dlang-bot · 2018-01-28T17:35:14Z

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

CyberShadow · 2018-01-28T17:39:05Z

std/json.d

 struct JSONValue
 {
-    import std.exception : enforceEx, enforce;
+    import std.exception : enforce, enforce;


Just one should be enough.

CyberShadow · 2018-01-28T17:48:38Z

Existing usage: https://github.com/search?q=enforceEx+language%3Ad&type=Code&utf8=%E2%9C%93

Related older change: #2462

CyberShadow · 2018-01-28T17:49:51Z

std/exception.d

+    import std.conv : ConvException;
+    alias enf = enforce!ConvException;
+    assertNotThrown(enf(true));
+    assertThrown(enf(false, "blah"));


How about assertThrown!ConvException ?

CyberShadow · 2018-01-28T17:50:16Z

std/exception.d

+@safe unittest
+{
+    import std.conv : ConvException;
+    alias enf = enforce!ConvException;


alias convEnforce = enforce!ConvException;

CyberShadow · 2018-01-28T17:51:11Z

std/exception.d

+// @@@DEPRECATED_2019-07@@@
 /++
+    $(RED Deprecated. Please use $(LREF enforce) instead. This function will be removed
+               in July 2019.)


Hmm, are we still doing deprecations by dates? I thought there was consensus that deprecating by D versions is overall better.

Ehm our deprecation process definitely needs some guidelines because I wasn't aware of this.
I would prefer D versions too.

By version would be nice. @MartinNowak what do you think?

I've been using month/year, because historically, our releases haven't been consistent enough to use any kind of release numbers when deprecating things. I can easily pick a month a year from now and put that in the message. Historically, that hasn't worked at all with versions. Martin has improved that situation considerably, but they're still not completely consistent. And as a user, it's far easier to look at a date and see about when the symbol is going away than it is to try and figure out when on earth a specific version is going to be released. If the date is a year from now, then it's clear that I have a year. If it's a version number, then it's not at all clear how long I have. I'd have to examine what the release schedule has been and estimate about when the symbol is going away.

However, the date in this case should be more like 2019-02, since it's supposed to be about a year from now (after which, it will be undocumented and have a non-ddoc comment indicating a date about a year from then for when it will be fully removed).

And as a user, it's far easier to look at a date and see about when the symbol is going away than it is to try and figure out when on earth a specific version is going to be released.

Err, sorry, I think that's a bit removed from reality. Essentially that's only true (and useful) if you're always using the latest version of DMD, and use nothing but the latest version of DMD. While this is mostly true for us who contribute to DMD and core D components frequently, it likely isn't for most D users.

Using dates is less useful for answering these questions:

Upgrading to which version of DMD will break my code?

Downgrading to which version of DMD will get my code to build again?

In which version of LDC or GDC will this code stop working?

Will upgrading to the next release of my Linux distribution mean I'll have fix my code?

Essentially, dates become a lot less useful in any situation where the user is not using the latest version of Phobos, whether that's thanks to a rolling release Linux distro, using D-apt, or religious frequent manual updates. In all other cases (Windows, tarballs, dlang.org distro packages, install script, builds from git), I believe version numbers would be more useful and easier to look up.

Well, technically, it keeps working for about a year after it's removed from the docs, so I'd be inclined to argue that the imprecision with regards to how it matches up with ldc releases or whatnot doesn't matter much, and the site always has the latest docs only, but if we get to the point that we have the docs for previous releases like we've discussed before, then dates definitely become worse.

So, fine, if you guys really want version numbers, then we go with version numbers, but it's going to make the actual time frame of deprecations more inconsistent, since it'll be harder to target a year ahead, but while they've usually been in the ballpark of where they should be, they don't always get moved along quite when they should anyway.

In that case, let's change it to say

// @@@DEPRECATED_2.084@@@

and

This function will be removed in 2.084.

and I'll have to create some PRs to fix the existing deprecations.

since it'll be harder to target a year ahead,

I don't think we need to try to target deprecations using calendar time, just use "N major versions ahead" instead of "N months ahead".

but while they've usually been in the ballpark of where they should be, they don't always get moved along quite when they should anyway.

Yeah, one advantage of using dates is that when we don't honor our own deprecation deadlines, it's easier to excuse that for dates than version numbers.

And as a user, it's far easier to look at a date and see about when the symbol is going away than it is to try and figure out when on earth a specific version is going to be released.

Thinking more on this, I think I understand better what you meant here.

If you are maintaining an open-source project (especially a library), a deprecation date (on a symbol used in your code) will tell you what's the latest point in time that the deprecation is guaranteed to still work for everyone, everywhere, so you don't need to really worry about it until then. Anything past that date, though, and there appears the chance that someone, somewhere is going to start having trouble building your software.

So... I'd say version numbers are more useful if you're writing code for yourself or your company, and deprecation dates are more useful if you're writing open-source software.

In that case, let's change it to say

OK. Changed. We should probably add a short documentation in the wiki or as a markdown document this repo about the deprecation process.

wilzbach · 2018-01-28T17:54:12Z

Existing usage: https://github.com/search?q=enforceEx+language%3Ad&type=Code&utf8=%E2%9C%93

Thanks! Though this PR doesn't depend on a decision on whether is worthwhile to deprecate this as it's just the preparation to make enforce equal to enforceEx.
Also if the potential breakage is a real concern here, we have multiple options here that improve the status quo without any cost of breakage:

just mark enforceEx as deprecated in the docs
just remove enforceEx from the docs
trigger a deprecation warning, but never follow through with the removal (preferred)

CyberShadow · 2018-01-28T17:58:27Z

This case doesn't seem too different from the others in which we've deprecated then removed Phobos symbols, so I see no reason to not stick to the current policy. It's also a super-easy fix, and it's going to be backwards-compatible through a few releases.

andralex · 2018-01-28T20:20:48Z

std/exception.d

+
+    T enforce(T)(T value, lazy const(char)[] msg = null,
+    string file = __FILE__, size_t line = __LINE__)
+    if (is(typeof({ if (!value) {} })))


We have ifTestable, though I have to say this is nicer.

Yes, ifTestable is one of those one-liners that cost a lot more than they are worth it:

phobos/std/traits.d

Line 7996 in 3f30d5c

enum ifTestable(T, alias pred = a => a) = __traits(compiles, { if (pred(T.init)) {} });

Interestingly it crept into Phobos with other one-liners: #3395

andralex · 2018-01-28T20:21:20Z

std/exception.d

+// @@@DEPRECATED_2019-07@@@
 /++
+    $(RED Deprecated. Please use $(LREF enforce) instead. This function will be removed
+               in July 2019.)


By version would be nice. @MartinNowak what do you think?

enforce can't fully replace enforceEx, hence it needs to stick around for at least one more release so that we can have a release where enforce can fully replace enforceEx. That way, someone can build their code with both the latest release and master without getting a bunch of deprecation messages.

wilzbach · 2018-02-02T14:51:29Z

(added a changelog entry)

Anything else needed for this PR? It uses a version for the deprecation now and once @JackStouffer's deprecation DIP has been approved we will have to update all the existing deprecations anyways.

It would be great to get this in before 2.079, s.t. enforce is fully capable of replacing enforceEx and we can start the deprecation cycle with 2.080.

wilzbach requested review from CyberShadow, andralex, kyllingstad and schveiguy as code owners January 28, 2018 17:35

wilzbach force-pushed the enforceEx branch from 59db6a4 to 7687161 Compare January 28, 2018 17:37

CyberShadow reviewed Jan 28, 2018

View reviewed changes

wilzbach force-pushed the enforceEx branch from 7687161 to 50171ab Compare January 28, 2018 17:40

CyberShadow approved these changes Jan 28, 2018

View reviewed changes

Turn enforce into an eponymous template + undocument enforceEx

2d6fb4f

wilzbach force-pushed the enforceEx branch from 50171ab to 3057268 Compare January 28, 2018 17:58

andralex approved these changes Jan 28, 2018

View reviewed changes

wilzbach force-pushed the enforceEx branch from 3057268 to d71df31 Compare January 31, 2018 06:35

wilzbach mentioned this pull request Jan 31, 2018

End the deprecation period of using the result of a comma expression dlang/dlang.org#2151

Merged

CyberShadow mentioned this pull request Jan 31, 2018

Add Deprecation Process DIP dlang/DIPs#108

Merged

Replace enforceEx with enforce

78dcc91

wilzbach force-pushed the enforceEx branch from d71df31 to 78dcc91 Compare February 2, 2018 14:50

andralex approved these changes Feb 6, 2018

View reviewed changes

andralex added the Merge:auto-merge label Feb 6, 2018

dlang-bot merged commit 757ec92 into dlang:master Feb 6, 2018

wilzbach mentioned this pull request Feb 6, 2018

Added -Xi to include new fields in JSON output dlang/dmd#7838

Merged

wilzbach deleted the enforceEx branch February 6, 2018 11:00

This was referenced Feb 6, 2018

Revert "Turn enforce into an eponymous template + undocument enforceEx" #6130

Merged

Turn enforce into an eponymous template + undocument enforceEx #6134

Merged

CircleCi: deactivate the host environment before running the testsuite dlang/dmd#7858

Merged

This was referenced Mar 16, 2018

Push deprecation of std.exception.enforceEx #6286

Merged

Simplify implementation of errnoEnforce #6289

Merged

Uh oh!

Comments

Conversation

wilzbach commented Jan 28, 2018

Uh oh!

dlang-bot commented Jan 28, 2018

Bugzilla references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyberShadow commented Jan 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyberShadow Jan 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jan 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyberShadow commented Jan 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Feb 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CyberShadow Jan 28, 2018 •

edited

Loading

wilzbach commented Jan 28, 2018 •

edited

Loading

CyberShadow commented Jan 28, 2018 •

edited

Loading