Use named tuple for std.array.byPair

[John-Colvin]

Not sure why it wasn't done this way originally

[DmitryOlshansky]

Is a breaking change unfortunately

[John-Colvin]

Any change to any return type is a breaking change*. If that's a blocker, then that's going to be a real source of stagnation...

*Actually, given D's powerful introspection, I'd wager a lot of PRs that people don't consider as breaking are strictly speaking breaking changes. Everything about the signature of a function can be introspected...

[wilzbach]

AFAICT one of the reason behind using Voldemort types was that a library can change the type and implementation without the user needing to care about it.
In this case it's even still a tuple (just not anonymous anymore) and no explicit type is stated by the API, so I can't imagine that this will cause any breakage.

@DmitryOlshansky if you worry about, we could add a changelog entry (or Bugzilla issue) for this change, s.t. it's visible on the changelog.

[wilzbach]

FWIW this could have also been simply:

return aa.byKeyValue.map!(pair => tuple!("key", "value")(pair.key, pair.value));

[John-Colvin]

Thanks, I always forget that the free function takes the names separately (which is funny because I worked a lot on that stuff getting Tuple.rename in)

[quickfur]

It wasn't done this way originally because of my negligence. Thanks for fixing it.

As for it being a breaking change, I disagree. The function is explicitly declared to return auto, meaning that the return type may change if the implementation changes. User code, therefore, should not be hard-coding the return type, but should be using typeof to obtain the correct type (e.g., if they wish to store it somewhere, like in a struct). If any code is hardcoding the return type to Tuple!(Key,Value), that code is technically wrong and ought to be broken.

[wilzbach]

Ping @DmitryOlshansky for responding to our feedback as expect for you comment there is nothing blocking a merge.

[DmitryOlshansky]

It may seem like a minor thing but consider usage where you need to store e.g. sorted array of pairs somewhere. Tuple!(Key,Value)[] arr; //fine Tuple!(Key, "key", Value, "value")[] arr; // do I have to remember all these literals? Anyway you could merge it but add it to changelog as a breaking change it is.

…

7 июня 2017 г., в 6:11, Sebastian Wilzbach ***@***.***> написал(а): Ping @DmitryOlshansky for responding to our feedback as expect for you comment there is nothing blocking a merge. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[quickfur]

Hmm you're right. This could be troublesome. Unless there was a way we could allow assignment to a Tuple with identical field types though not necessarily the same names. Might be tricky with the current implementation, though.

[John-Colvin]

That wouldn't help when assigning to an array like in Dmitry's example would it?

[wilzbach]

Anyway you could merge it but add it to changelog as a breaking change it is.

I'm still for this as imho the implementation shouldn't really depend on this and showing a warning in the changelog should be good enough.
@quickfur @JackStouffer @ZombineDev - what's your take on this. Agree or veto?

@John-Colvin: could you please append an entry if other reviewers agree on this?

[JackStouffer]

I think this change is fine with an accompanying changelog entry

[JackStouffer]

Let's move on with this. I'll add the changelog entry myself.

[wilzbach]

@JackStouffer the error is due to the DUB registry being down:

Root package assert_writeln_magic references unknown package libdparse
posix.mak:625: recipe for target '.generated/assert_writeln_magic' failed

[dlang-bot]

Thanks for your pull request, @John-Colvin! 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

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.

[wilzbach]

@JackStouffer I rebased this PR to rekick DAutoTest and while I was at it, I added a changelog entry as well.