Skip to content

Revive std.range.slide from the dead#5943

Merged
dlang-bot merged 1 commit intodlang:masterfrom
wilzbach:slide-reboot
Jan 17, 2018
Merged

Revive std.range.slide from the dead#5943
dlang-bot merged 1 commit intodlang:masterfrom
wilzbach:slide-reboot

Conversation

@wilzbach
Copy link
Contributor

This is a revival of std.range.slide ... finally!

History

std.range.slide was introduced in #4027 and I delayed its release in #5614 as at the time of the merge I didn't have to work on it.

Why was it needed to rework this function before its release?

tl;dr: the implementation of withFewerElements was broken for Yes.withFewerElements.
(see #5614 for details).

Changes

It's hard make an exact summary of the changes,

  • added proper handling for Yes.withPartial
  • renamed withFewerElements to withPartial (shorted & clearer name)
  • made all tests function use withPartial explicitly
  • extensive tests (2/3 of the size)
  • more comments and explanation (another big chunk of this functions size)

I apologize for this diff being so big, but most changes are the extended test coverage.

@dlang-bot
Copy link
Contributor

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.

Copy link
Contributor

@JackStouffer JackStouffer left a comment

Choose a reason for hiding this comment

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

One thing I would like to see before this gets merged is to cut down on the number of tests, as this is most likely the most tested range function by far. There are tons of places here where many different cases are tested where one or two would have sufficed.

@wilzbach
Copy link
Contributor Author

wilzbach commented Jan 7, 2018

One thing I would like to see before this gets merged is to cut down on the number of tests, as this is most likely the most tested range function by far.

Removed the number of tests by 300 lines (and addressed your other points).

@JackStouffer
Copy link
Contributor

Looks good. I'll wait a week to allow other commentary and merge if there's no comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants