Fix Issue 17525 - std.algorithm.searching.skipOver should have a single argument with pred version

[RazvanN7]

No description provided.

[dlang-bot]

Thanks for your pull request, @RazvanN7! 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
✓	17525	std.algorithm.searching.skipOver should have a single argument with pred version

[RazvanN7]

Any ideas on why circleci is failing? The line that is reported doesn't even call empty

[wilzbach]

#5497

[wilzbach]

You need to import empty from std.range.primitives here (public tests need to be runnable standalone)

[RazvanN7]

@wilzbach Ah, didn't see that one. Thanks anyway.

[wilzbach]

Ah, didn't see that one

As this is now the second PR in a row, you should start a NG thread to remind people to tag their PRs. In theory the bot could do this as well (I don't recall why it doesn't, feel free to open an issue)

[RazvanN7]

@wilzbach should I keep this PR open or should I close it and review @JackStouffer 's ?

[wilzbach]

@wilzbach should I keep this PR open or should I close it and review @JackStouffer 's ?

As the other PR has a rather long discussion, I would first read through it and try to find out the consensus. IIRC there wasn't none yet. Once there is an agreement, you can always reopen this PR if necessary (or create a new one from the same branch). Sounds good?

[wilzbach]

Next time please add "Fix Issue $Number" so that the dlang bot can associate your PR with the bug fix.

Btw do you see the ✗ on the bug list summary? It means the issue is just "mentioned" and won't be automatically closed.
I have opened a PR to clarify this -> dlang/dlang-bot#124

[RazvanN7]

@wilzbach Oh I get it now. I didn't thought at all what that tiny x means. I rebased and added the proper name. Thanks for all the enlightening comments!

[andralex]

OK, please mind @wilzbach 's review and then let's get this in. Thanks!

[RazvanN7]

@andralex it's there: https://github.com/dlang/phobos/pull/5543/files#diff-8b73351c2fbfb7fef513b00ced46d022R3989 , but I amended the commit so that there is no need to squash

[CyberShadow]

Err, I'm certain I must have pointed out somewhere (not this thread I guess) that this implementation is inconsistent with the other skipOver overloads. This one loops, whereas the others don't.

@andralex Does this make sense to you?

[RazvanN7]

@CyberShadow What do you mean by loop? https://github.com/dlang/phobos/pull/5543/files#diff-8b73351c2fbfb7fef513b00ced46d022R3947 : this seems like a loop to me

[CyberShadow]

That loop simply consumes the two input ranges for comparison; it does not consume multiple instances of the "prefix". Consider the difference between s.skipOver(' ') and s.skipOver!(c => c == ' ') - intuitively one may think that these should behave the same, but they don't!

[RazvanN7]

@CyberShadow As I see it, there are 2 classes of skipOver methods: those that skip over multiple range elements and those that skip over the first element. This PR enhances the first category.

The 2 cases you are taking about are likely to appear in the real world and skipOver should manage them both. I don't see any inconsistency here.

[andralex]

@CyberShadow it's a point, but the usefulness in parsing is there.

• s.skipOver(' ') ---> "skip over one space"
• s.skipOver(c => c == ' ') ---> "skip over any number of spaces"
• s.skipOver(c => c.among(' ', '\t', '\r', '\n') ----> "skip all whitespace"

[CyberShadow]

It would be less confusing if there was a different overload set for functions that skip over multiple instances of the pattern.

[CyberShadow]

skipOver:

• s.skipOver(' ') ---> "skip over one space"
• s.skipOver(c => c.among(' ', '\t', '\r', '\n') ----> "skip over one whitespace character"

skipWhile:

• s.skipWhile(' ') ---> "skip over spaces"
• s.skipWhile(c => c.among(' ', '\t', '\r', '\n') ----> "skip all whitespace"

The return types should be different. The version that skips multiple prefixes ought to return a size_t.

[andralex]

@CyberShadow that's awfully nice, @RazvanN7 how about implementing that real quick?

[CyberShadow]

@andralex @RazvanN7 Found the previous discussion: #5497 (comment)

Looks like we already have some primitives in Phobos to do some of these things. CC @wilzbach