Skip to content

Move various deprecations along.#1766

Merged
MartinNowak merged 1 commit intodlang:masterfrom
jmdavis:deprecations
Dec 19, 2013
Merged

Move various deprecations along.#1766
MartinNowak merged 1 commit intodlang:masterfrom
jmdavis:deprecations

Conversation

@jmdavis
Copy link
Member

@jmdavis jmdavis commented Dec 12, 2013

I'm may have missing some that didn't end up with the exact comments that I grep for, but I did manage to catch several that had been missing those comments.

@Geod24
Copy link
Member

Geod24 commented Dec 12, 2013

I suppose you followed https://github.com/D-Programming-Language/druntime/blob/master/HACKING.md#deprecation-process ?
This deprecation process was very hard to find: this file is the only place it's documented. Shouldn't it be in http://wiki.dlang.org/Development_and_Release_Process as well ?

What about crc32.d ? It has been deprecated for over an year now. is there a reason it's not /won't be removed ?

@jmdavis
Copy link
Member Author

jmdavis commented Dec 12, 2013

@Geod24 I don't know anything about that page and am not aware of any official documentation on deprecation, but I'm well aware of how we do it, because I'm the one who handles most of it. Previously, it was 6 months marked as "scheduled for deprecation," 6 months as actually deprecated, and then it was removed. However, now that we have deprecation messages and deprecated results in a warning rather than an error, it probably makes more sense to just mark as deprecated for a year without the 6 months of being documented as "scheduled for deprecation." Also, Andrei pushed to have stuff left in but undocumented for at least a little while instead of being removed. So, I've started leaving stuff deprecated and undocumented for 6 months after "removal" before actually being removed.

So, in general, at this point, I believe that what we're looking at is

  1. deprecate with a message and document that it will be removed in one year - typically with a ddoc comment such as $(RED Deprecated. This function will be removed in December 2014. Please use foo.bar instead.).
  2. Once the date in the comment is reached, remove the symbol's documentation and the mark it with a regular comment such as Explicitly undocumented. It will be removed in July 2015 where the date is approximately 6 months after the ddoc comment is removed.
  3. Once the date in the comment is reached, completely removed the symbol.

I try and use those specific comments so that I can grep for them, but they don't always get used, so sometimes they get missed and don't get moved to the next stage of the deprecation process on time, and sometimes I forget to check for symbols that are scheduled to be moved to the next step in the deprecation process. But being a bit behind on moving stuff along generally isn't a big deal.

I didn't necessary catch everything in this pull, though I tried to catch all of the deprecated symbols that didn't have the greppable comments on them. As for crc32.d specifically, it does look like I missed it (in part because it wasn't even in std, and I forgot that any module existed outside of any package like that). But I don't want to mess with any file removals in this pull. I can get that in a future pull.

@monarchdodra
Copy link
Collaborator

LGTM.

@MartinNowak
Copy link
Member

Great, please rebase.

@monarchdodra
Copy link
Collaborator

Pinging 4 rebase.

@jmdavis
Copy link
Member Author

jmdavis commented Dec 19, 2013

I'll get to this in the next few days. I'm stuck working a bunch of overtime to meet a deadline, so I've been time crunched, or I would have done it already.

@monarchdodra
Copy link
Collaborator

Make sure you ping us when its done, lest it de-syncs again :)

MartinNowak added a commit that referenced this pull request Dec 19, 2013
Move various deprecations along.

Conflicts:
    std/algorithm.d
    std/zip.d
@MartinNowak MartinNowak merged commit efd6ea0 into dlang:master Dec 19, 2013
@jmdavis
Copy link
Member Author

jmdavis commented Dec 20, 2013

Wait, how did this get merged without me doing a rebase?

@yebblies
Copy link
Contributor

I assume Martin edited it in the merge commit. @MartinNowak, you probably shouldn't do that. The edited code doesn't get reviewed, or put through the autotester. Very little is urgent enough it can't wait for a rebase.

@MartinNowak
Copy link
Member

Maybe I should be more patient, but for trivial merge conflicts it avoids communication ping-pong and sometimes pull requests get abandoned because of this (not by you @jmdavis).
Here I resolved 2 merge conflicts and I was involved in both conflicting phobos changes.
If you use something else than github (gitk) you can easily see the changes.
It's these two:
c445f1c#diff-ff74a46362b5953e8c88120e2490f839R10589
c445f1c#diff-eaa2330fae11d67b27c5750409038ef5R97

@JakobOvrum
Copy link
Contributor

Here I resolved 2 merge conflicts and I was involved in both conflicting phobos changes.

It looks like you've erased the original author this way, which is problematic when using tools like git-blame and git-log, and might be discouraging to some people.

edit:

Nevermind, that's not true; I just didn't see it easily on Github because it shows up at its original date.

@yebblies
Copy link
Contributor

My main concern is that the changes no longer match exactly with what is in the pull request. Merges don't always come out correctly, and this makes it easier for mistakes to slip through unnoticed. If a pull request is actually abandoned then that's a different issue, and thankfully a lot less of a problem than it used to be.

@yebblies
Copy link
Contributor

I'd rather this didn't happen to my pull requests. Maybe it's the fact that Walter used to heavily edit my pull requests when merging that makes me dislike it.

We should come up with an official policy on this. I would honestly prefer if pull requests were always merged with the the autotester's auto-merge, except in exceptional circumstances.

@MartinNowak
Copy link
Member

It's not that I like to do this, but currently we have 109+23+79 open pull requests and most of them simply rot.
My personal time is heavily restricted so sometime I do this for minor edits like tabs, indentation, spelling or minor merge conflicts which would otherwise require another ping-pong round.
I'm a little biased myself.

@andralex
Copy link
Member

@MartinNowak: it would be awesome if we figured a policy for addressing the oldest pull requests.

@ghost
Copy link

ghost commented Jan 7, 2014

P.S. @CyberShadow (and others too): to avoid us from getting confused from messages that disappear you can instead use a <del></del> section, e.g.:

Hi, this pull deprecated std.algorithm.canFind. The deprecation message says to use any instead. However, any doesn't have a default predicate, whi…

@CyberShadow
Copy link
Member

Sorry. I need to learn to double-check my conclusions before posting. This is the third time this week!

@ghost
Copy link

ghost commented Jan 7, 2014

Well if it's actually something newbies will run into not knowing how to use the alternative to canFind then it's an opportunity to improve the docs.

@CyberShadow
Copy link
Member

Here's why I messed up this time:

In DustMite, there was this line of code:

    if (canFind!((a){return !match(f.filename, a).empty;})(noRemove))

This was generating a deprecation error. I looked at std.algorithm, and noticed that an overload of canFind was deprecated. However, I did not realize that only the overload that didn't take any additional parameters (which is, indeed, synonymous to any) was deprecated. The reason is that the DustMite code, which I wrote a while ago (before lambda syntax even, apparently), did not use UFCS syntax - today I would always use UFCS for range calls, so I didn't notice that there wasn't anything before "canFind".

@monarchdodra
Copy link
Collaborator

Regarding, canFind, to be honest, I'm not sure why it's deprecated. sure any will do the same thing, but I find it weird that canFind!pred(a) to be illegal, when find!pred(a) is legal.

I think it's a gratuitous deprecation.

@jmdavis jmdavis deleted the deprecations branch June 20, 2014 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants