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

Don't export reindex and substrides #31201

Merged
merged 2 commits into from
Mar 1, 2019
Merged

Don't export reindex and substrides #31201

merged 2 commits into from
Mar 1, 2019

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Feb 28, 2019

These were inadvertently exported by the deprecate macro in #30789. Both are tailored to SubArray and don't check for preconditions that are enforced by the SubArray constructor. While we could perhaps export them, I'd like to do so intentionally... and add friendlier errors for violations of these preconditions.

It was inadvertently exported by #30789.  It's tailored to `SubArray` and doesn't check for preconditions that are enforced by the `SubArray` constructor — so while we could export it, I'd like to do so intentionally and add friendlier errors for violations of these preconditions.
@StefanKarpinski
Copy link
Member

We should maybe do a review of all added exports before each release.

@mbauman
Copy link
Member Author

mbauman commented Feb 28, 2019

Once we get #26919 and #31202 down to a manageable level, we could make having no undocumented exports a blocking step in the release process script. That's how I discovered these.

@StefanKarpinski
Copy link
Member

That's even better! We should make a sprint to get those finished up so that we can require documentation of exports. There's still a slight danger, however, of accidentally exporting something since we do sometimes write docstrings on internal bindings.

@fredrikekre fredrikekre merged commit 9329e0f into master Mar 1, 2019
@fredrikekre fredrikekre deleted the mb/unindexreindex branch March 1, 2019 11:10
@KristofferC
Copy link
Member

Yes, even though we document a bunch of exported stuff now we should imo stiull to an audit to check what things should perhaps be unexported in 2.0.

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.

4 participants