Skip to content

num: remove pub from sub-crates - #190

Merged
homu merged 1 commit into
masterfrom
private-crates
May 12, 2016
Merged

homu merged 1 commit into
masterfrom
private-crates

Conversation

@cuviper

@cuviper cuviper commented Apr 29, 2016

Copy link
Copy Markdown
Member

Rust 1.4 through 1.7 will issue warnings that pub extern crate does
not work as expected, although it appears to be fine. But Rust 1.8 and
later issue #[warn(private_in_public)] for pub use num_foo as foo if
that crate isn't public, and that's headed toward a hard error.

@bluss suggested instead pub mod foo { pub use num_foo::*; }, which
I thought I had tried before, but it appears to work fine on all
versions of Rust. Let's do it!

A small downside is that docs for num::foo now just show the wildcard
reexport, instead of direct documentation, but at least there's a link
to follow to the sub-crate.

It's a breaking change that num::num_foo paths are no longer public,
but we didn't really want those exposed in the first place. I consider
this minor -- people should either use the num::foo module as before
the split-up, or use the num_foo crate directly.

Fixes #189.

Rust 1.4 through 1.7 will issue warnings that `pub extern crate` does
not work as expected, although it appears to be fine.  But Rust 1.8 and
later issue `#[warn(private_in_public)]` for `pub use num_foo as foo` if
that crate isn't public, and that's headed toward a hard error.

@bluss suggested instead `pub mod foo { pub use num_foo::*; }`, which
I thought I had tried before, but it appears to work fine on all
versions of Rust.  Let's do it!

A small downside is that docs for `num::foo` now just show the wildcard
reexport, instead of direct documentation, but at least there's a link
to follow to the sub-crate.

It's a breaking change that `num::num_foo` paths are no longer public,
but we didn't really want those exposed in the first place.  I consider
this minor -- people should either use the `num::foo` module as before
the split-up, or use the `num_foo` crate directly.

Fixes #189.
@bluss

bluss commented Apr 29, 2016

Copy link
Copy Markdown
Contributor

I only mentioned it exists, don't know if it will work well.

@cuviper

cuviper commented Apr 29, 2016

Copy link
Copy Markdown
Member Author

That's partly why I opened a PR instead of pushing this right away. It looks fine so far.

@cuviper

cuviper commented May 12, 2016

Copy link
Copy Markdown
Member Author

@homu r+

@homu

homu commented May 12, 2016

Copy link
Copy Markdown
Contributor

📌 Commit 865c491 has been approved by cuviper

@homu
homu merged commit 865c491 into master May 12, 2016
homu added a commit that referenced this pull request May 12, 2016
num: remove `pub` from sub-crates

Rust 1.4 through 1.7 will issue warnings that `pub extern crate` does
not work as expected, although it appears to be fine.  But Rust 1.8 and
later issue `#[warn(private_in_public)]` for `pub use num_foo as foo` if
that crate isn't public, and that's headed toward a hard error.

@bluss suggested instead `pub mod foo { pub use num_foo::*; }`, which
I thought I had tried before, but it appears to work fine on all
versions of Rust.  Let's do it!

A small downside is that docs for `num::foo` now just show the wildcard
reexport, instead of direct documentation, but at least there's a link
to follow to the sub-crate.

It's a breaking change that `num::num_foo` paths are no longer public,
but we didn't really want those exposed in the first place.  I consider
this minor -- people should either use the `num::foo` module as before
the split-up, or use the `num_foo` crate directly.

Fixes #189.
@hauleth
hauleth deleted the private-crates branch May 12, 2016 20:18
@hauleth

hauleth commented May 12, 2016

Copy link
Copy Markdown
Member

In Octavo I am using pub extern crate name as other_name; which has that advantage that this provide better documentation.

@cuviper

cuviper commented May 12, 2016

Copy link
Copy Markdown
Member Author

Sure, that would be nice, but it runs afoul of the same warning as #189 in older rustc.

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