Skip to content

Conversation

@dhasenan
Copy link
Contributor

https://issues.dlang.org/show_bug.cgi?id=18124 : explicit return types are good for documentation. Mildly annoying for editing when you are breaking the public API, but that shouldn't happen, right?

@dlang-bot
Copy link
Contributor

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

@JackStouffer
Copy link
Contributor

but that shouldn't happen, right?

Quite the contrary, because it's auto we can "break" the API as long as the interface remains the same, e.g. https://github.com/dlang/phobos/pull/5436/files

Thanks for the PR but I'm going to close this because this is churn which has little benefit and restricts us in the future. If you find the documentation lacking, please bug/PR it.

@dhasenan
Copy link
Contributor Author

The types are owned by std.regex; they aren't shared with other modules. They have no other purpose. So we can alter them however we wish without having to rename them.

If we needed to switch to a different return type that is shared by another module, it is easy to add an alias.

Finally, the argument that we can alter return types at any time if they're listed as auto applies to literally every function in phobos. What's so special about Capture and RegexMatch that we need to be able to swap them out?

@Temtaime
Copy link
Contributor

Even worse, such an explicit return type makes function signature looks ugly and any changes to it force us to modify it.
I see no benefit in such prs. Improve the documentation if you want.

@wilzbach
Copy link
Contributor

@dhasenan thanks a lot for your pull request and wanting to make Phobos a better library. However, I think you should put your energy in something new or real bugs instead.

this is churn which has little benefit and restricts us in the future

+1 (and I also think that auto reads itself a lot better in the documentation than RegexMatch!(Unqual!R)).

If we needed to switch to a different return type that is shared by another module, it is easy to add an alias.

No we wouldn't be able to:

  • we can't change the meaning of public types
  • just look at the Unqual's you used

If people really need the return type, it's as simple as typeof(regex("foo").

What's so special about Capture and RegexMatch that we need to be able to swap them out?

Nothing, but development is happening and why should we restrict ourselves without need?

dhasenan added a commit to dhasenan/phobos that referenced this pull request Dec 25, 2017
As per the discussion at dlang#5963,
we want the option to swap out the types for Regex, Capture, etc.
This is impossible if people are able to refer to Regex by name.
Furthermore, according to @wilzbach, aliases are not an option.
Therefore, this PR deprecates the public Regex alias, to allow us to
switch out the concrete type later.
dhasenan added a commit to dhasenan/phobos that referenced this pull request Dec 25, 2017
As per the discussion at dlang#5963, we
want the option to swap out the types for Regex, Capture, etc. This
is impossible if people are able to refer to Regex by name.
Furthermore, according to @wilzbach, aliases are not an option.
Therefore, this PR deprecates public uses of the Regex types to allow
us to switch out the concrete type later.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants