Skip to content
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

Restore accidentally removed import from std.math #7483

Merged
merged 1 commit into from
May 15, 2020

Conversation

Geod24
Copy link
Member

@Geod24 Geod24 commented May 15, 2020

It was removed in PR7473: #7473 (comment)
It caused a two-days-long outage of Buildkite: dlang/ci#418 (comment)

@Geod24 Geod24 requested a review from ibuclaw as a code owner May 15, 2020 14:28
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @Geod24!

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.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7483"

@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented May 15, 2020

Well, that is a private import ... that shouldn't be visible when importing std.math (?)

@Geod24
Copy link
Member Author

Geod24 commented May 15, 2020

It's a bug, selective imports create an alias. Some say it's a feature...

@Geod24
Copy link
Member Author

Geod24 commented May 15, 2020

My bad, the bug seems to have been fixed, but it was public anyway (L543). The import was on line 684.

@MoonlightSentinel
Copy link
Contributor

Thanks, did not see that public:.

Should this already deprecate the import?

@Geod24
Copy link
Member Author

Geod24 commented May 15, 2020

Should this already deprecate the import?

I think it should be deprecated, but I have no interest in deprecating that import myself.

I'm only raising this PR because the original reviewer 1) ignored the CI and 2) didn't act when the error started to creep up everywhere. If this import is to be deprecated, it should be by someone that has interest in spending the time to do it, not the person fixing someone else's careless mistake.

And IMO this reasoning stands regardless of the amount of work required. Deprecating an import might be just a few minutes of time, but it's a few minutes more than I was willing to spend on Phobos when I started to work on my own stuff. This is also why I prefer to outright revert stuff than attempt to fix the mistake, as you've seen recently.

We could call this the bug asymmetry principle, if you will.

@MoonlightSentinel
Copy link
Contributor

Fair enough

@dnadlinger
Copy link
Member

Apparently the DAutoTest result upload failed… Is there a better way these days than just to empty-amend the branch?

@wilzbach wilzbach merged commit 28ff71b into dlang:master May 15, 2020
@wilzbach
Copy link
Member

Nope unfortunately not, but in this case the force-merge option can be used to save some time.

@Geod24 Geod24 deleted the here-we-go-again branch May 16, 2020 04:23
@ibuclaw
Copy link
Member

ibuclaw commented May 17, 2020

@Geod24 - we're blocked by you. dlang/dmd#10534

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.

8 participants