Skip to content

Fix issue 18124 -- RegexMatch.front isn't clear as to what it returns #5976

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

Merged
merged 1 commit into from
Jan 3, 2018

Conversation

schveiguy
Copy link
Member

Easiest thing to do was explicitly type the result. It's probably less effort than documenting what it returns in ddoc, and is clearer to the user.

In all other cases in this file, the auto returns have explicit docs as to the return type. This really was the only anomaly.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @schveiguy!

Bugzilla references

Auto-close Bugzilla Description
18124 std.regex.RegexMatch's front property is under-documented

@quickfur
Copy link
Member

quickfur commented Jan 2, 2018

I'm not sure this is a good idea. It binds us to an explicit return type that we may not be able to change in the future, e.g., if for example a future implementation wants to return distinct types for matchFirst and matchAll (since the former only needs to encapsulate a single match whereas the latter a range of matches).

@schveiguy
Copy link
Member Author

It binds us to an explicit return type that we may not be able to change in the future

That ship has sailed, the return type is not a voldemort type, so code may already exists that uses the real type name. It's what I would do if I were stuffing the result into a struct.

@schveiguy
Copy link
Member Author

In fact, the docs of RegexMatch already state it's a forward range of Captures elements. This just clarifies it close to where you would be looking for the element type.

Copy link
Member

@quickfur quickfur left a comment

Choose a reason for hiding this comment

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

Fair enough, let's just go with this then.

Copy link
Member

@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.

@schveiguy Please see #5963

I think it's better to just update the docs

@JackStouffer
Copy link
Member

Wait, I apologize. I didn't see your other comments before I submitted the review.

@dlang-bot dlang-bot merged commit 9012cb4 into dlang:master Jan 3, 2018
@schveiguy schveiguy deleted the fix18124 branch January 3, 2018 11:18
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.

5 participants