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

Issue 17224 - Foreach documentation still refers to TypeTuples, rather than AliasSequences #5484

Merged
merged 2 commits into from
Jun 28, 2017

Conversation

wilzbach
Copy link
Member

Analogous to dlang/dlang.org#1701

Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM too.

@dlang-bot
Copy link
Contributor

dlang-bot commented Jun 18, 2017

Thanks for your pull request, @wilzbach! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Fix Bugzilla Description
17224 Foreach documentation still refers to TypeTuples, rather than AliasSequences

@@ -164,7 +164,7 @@
*/
module std.traits;

import std.typetuple; // TypeTuple
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like Jenkins fails, because this somehow publicly imported functions like staticMap, which is now not the case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CyberShadow can you confirm if that's the case with ae?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

staticMap was originally in std.traits, until 5c9725d moved it to another module. So, breaking this would require going through a deprecation cycle.

Sidenote: ae was added to the project tester just a few days ago, I'm happy it caught a potential regression already :)

wilzbach added a commit to wilzbach/phobos that referenced this pull request Jun 20, 2017
@wilzbach
Copy link
Member Author

staticMap was originally in std.traits, until 5c9725d moved it to another module. So, breaking this would require going through a deprecation cycle.

Hmm I can only see staticMap and allSatisfy on this PR - is there more hidden legacy inheritance?

std/traits.d Outdated

// Legacy inheritance from std.typetuple - should be deprecated
// See also: https://github.com/dlang/phobos/pull/5484#discussion_r122602797
public import std.meta : staticMap;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add a deprecation message as soon as we have figured out if just staticMap or more symbols are affected.

Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, @CyberShadow?

@CyberShadow
Copy link
Member

I think Sebastian was going to work on this some more? As per his comment:

I will add a deprecation message as soon as we have figured out if just staticMap or more symbols are affected.

Otherwise, if it doesn't break the project tester, it's good in my book.

@PetarKirov
Copy link
Member

Sorry, I missed that.

@wilzbach
Copy link
Member Author

I think Sebastian was going to work on this some more? As per his comment:

I will add a deprecation message as soon as we have figured out if just staticMap or more symbols are affected.

Yep I wanted to wait and see if someone comes forward with more symbols that should be publicly imported in std.traits. Nothing happened -> I think we can move forward here :)

However, it seems that dmd doesn't trigger the deprecation warnings. Any ideas / workarounds? (I already tried using an alias symbol instead of the selective public import).

Sample programs:

void main(string[] args)
{
    import std.traits;
    alias TL = staticMap!(Unqual, int, const int, immutable int);

    import std.meta : AliasSeq;
    static assert(is(TL == AliasSeq!(int, int, int)));
}

or one without std.meta:

void main(string[] args)
{
    import std.traits;
    alias TL = staticMap!(Unqual, int, const int, immutable int);

    import std.stdio;
    foreach (el; TL)
    {
        writeln(el.stringof);
    }
}

@JackStouffer
Copy link
Member

@wilzbach How about leaving the deprecations to a different PR so this can be moved forward?

@wilzbach
Copy link
Member Author

@wilzbach How about leaving the deprecations to a different PR so this can be moved forward?

Good idea. Done.

@dlang-bot dlang-bot merged commit 32b5334 into dlang:master Jun 28, 2017
wilzbach added a commit to wilzbach/phobos that referenced this pull request Jul 20, 2017
@wilzbach wilzbach deleted the fix-17224 branch December 11, 2017 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants