Undocument public structs in std.range

[wilzbach]

From #5624 (review)

due to the unfortunate historical accident that Chunks was exposed as a public API rather than encapsulated as a Voldemort or private module-global struct.

Btw two simple things that we could do to improve the status quo:
kill off the autoindex (which is very ugly anyway)

I have already done this, but it's not visible yet because 2.075.0 hasn't been released yet.
See: https://dlang.org/phobos-prerelease/std_range.html

undocument the publicly exposed structs, s.t. at least the docs look nicer for newcomers

-> This PR (I left SortedRange public)

We should wait until DAutoTest is working here.

[dlang-bot]

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.

[wilzbach]

Pull request SHA mismatch

Yeah

dautotest: Git    commit SHA1: 172414c4bc752bfa1d3792853032d1bb21479a68
dautotest: GitHub commit SHA1: d101711b50fa42b6f6e65184f49a2cc324c8faaa

I can observe the following

• d101711 is my local commit
• auto-tester sends the following to GH:

2017-07-17T23:14:20.851043+00:00 app web.1 - - [3953EF75:33ECF975 dbg] github/pull_request: state=pending, sha="172414c4bc752bfa1d3792853032d1bb21479a68", url="https://auto-tester.puremagic.com/pull-history.ghtml?projectid=14&repoid=37&pullid=5625"
...
2017-07-18T00:08:05.152415+00:00 app web.1 - - [3953EF75:39557775 dbg] github/pull_request: state=pending, sha="d101711b50fa42b6f6e65184f49a2cc324c8faaa", url="https://auto-tester.puremagic.com/pull-history.ghtml?projectid=14&repoid=37&pullid=5625"

• I can only find d101711 in the logs of status hooks from auto-tester and your DAutoTest

This all started roundabout the time that we added the CODEOWNERS file and as this seemed to be very buggy and considering that a major outage would have been noticed by more people, maybe it's due to some bug in their CODEOWNER script that they don't send the correct payload (or don't send it all)?

[CyberShadow]

This all started roundabout the time that we added the CODEOWNERS file and as this seemed to be very buggy and considering that a major outage would have been noticed by more people, maybe it's due to some bug in their CODEOWNER script that they don't send the correct payload (or don't send it all)?

My theory is:

• Many CI services use a different method to fetch the PR head, such as fetching from the user's GitHub fork instead of the refs/pull/###/head refs
• Those CI services which do use the refs/pull/###/head refs, do not verify that the SHA they fetch matches the SHA the GitHub API promised.

Although I could make DAutoTest try to use another method to fetch the PR (TBH it would be really annoying since it would require a bunch of layering violations due to an operation previously requiring only git now also requiring access to the GitHub API incl. auth and caching and all that), it won't fix the fact that some of our CI services may still be testing the wrong commit.

Still no reply from GH support after 23 hours, FWIW. Maybe someone higher (Andrei/Walter) could try pinging them.

[MartinNowak]

Looks like a race condition, no? You added another commit and the status arrived before github updated their PR commit internally. Could happen when the CIs directly use git to fetch the PR branch instead of checking out the commit of the received webhook.
As you directly push to their git subsystem, I'd assume that is updated right away while their metadata dbs and hooks are likely updated asynchronously (and might have a backlog/delay).

[CyberShadow]

Looks like a race condition, no? You added another commit and the status arrived before github updated their PR commit internally.

That's what I thought when it happened the first time, but it's persistent.

Could happen when the CIs directly use git to fetch the PR branch instead of checking out the commit of the received webhook.

It's not possible to fetch a specific commit from a remote, only a ref.

As you directly push to their git subsystem, I'd assume that is updated right away while their metadata dbs and hooks are likely updated asynchronously (and might have a backlog/delay).

So far we haven't seen the mismatch get unstuck just by sitting there for a while.

[MartinNowak]

So far we haven't seen the mismatch get unstuck just by sitting there for a while.

Right, looks like their head update failed altogether. Interestingly refs/pull/5625/merge contains the correct merge commit. Should I leave things for github support or can I update this PR?

[CyberShadow]

Should I leave things for github support or can I update this PR?

I don't know. In any case, we have no shortage of examples of this problem occurring across other PRs.

[JackStouffer]

Eh, I really don't see a reason to do this if we already have the new booktable in place.

[andralex]

It doesn't seem a shotgun approach is good here - perhaps more of a case basis attitude. The litmus test is, does one want to store such a range as a field in a class or struct? If so, having the name available may be better than using typeof to fetch it.

A related matter: if the results are undocumented, their name still appears as the return type of the functions. E.g. take still returns a Take!R and people won't be able to figure where that comes from.

What is the main motivation behind this?

[jmdavis]

E.g. take still returns a Take!R and people won't be able to figure where that comes from.

Take is actually one of those that really needs to have its own, public type and be properly documented. Without it, containers are kind of screwed when it comes to functions like remove when they take a range. They need to be able to take Take! of whatever the range type is for that container.

The others are more debatable, but when you're dealing with a function that's explicitly generating a range rather than one that's really doing some sort of algorithm that happens to use ranges, it's a lot more likely that being able to name the type is going to be useful.

So, I'm inclined to argue that we just leave these be. Certainly, Take needs to stay public and documented, and each of the others need to be examined individually if we're going to consider it. e.g. the nature of RefRange is such that it should probably be a documented, nameable type as well. I don't know about each of the others; I'd have to study them more. But given that they already have names, I'm inclined to just leave them as-is. Undocumenting them really doesn't do anything for us IMHO, and I don't think that they're worth deprecating. And if we're not planning on deprecating them, why not let folks continue to use them by name? As great as Voldemort types can be, sometimes it's actually really nice to be able to explicitly name range types, and the functions in std.range tend to be of the sort where it's sometimes useful to name them - if not borderline necessary (the result of take being the prime example of that).

[braddr]

What's the status here.. seems stalled.

[JackStouffer]

@braddr Three against one for. I was waiting for @wilzbach to give his arguments on this before doing anything.

[MetaLang]

I agree with @andralex and @jmdavis that some of them need to stay documented at this point.

[wilzbach]

Take is actually one of those that really needs to have its own, public type and be properly documented. Without it, containers are kind of screwed when it comes to functions like remove when they take a range. They need to be able to take Take! of whatever the range type is for that container.

typeof(take(myRange)) - the point was that the user shouldn't need to know the return type of any function in Phobos. On the contrary, (in theory) we should be allowed to change it.

What is the main motivation behind this?

Cleaning up the public docs.

As @JackStouffer has pointed out, with the new booktables, there's less of a reason for hiding the structs from the user, so I close this for now.

@ future: feel free to revisit this topic if it ever comes up again.

[PetarKirov]

@wilzbach Take is a public API used in std.container , for example: https://dlang.org/phobos/std_container_rbtree#.RedBlackTree.remove.2. Iif Take wasn't public (e.g. a Voldemort type) the function signatures would be much uglier.