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

Re-export public dependencies #5063

Merged

Conversation

cwfitzgerald
Copy link
Member

@cwfitzgerald cwfitzgerald commented Jan 15, 2024

Closes #5054.

Useful enough to have, and doesn't clutter things too much.

This re-organizes our naga dependency a bit, notably that it's now properly optional on wasm, only enabled if you enable a feature that requires transpilation, or a direct mention in our public interface.

image

@cwfitzgerald cwfitzgerald requested a review from a team as a code owner January 15, 2024 05:17
@cwfitzgerald cwfitzgerald force-pushed the re-export-public-dependencies branch from b76e6c1 to a3dfbe3 Compare January 15, 2024 07:11
@cwfitzgerald
Copy link
Member Author

@Wumpf @daxpedda could you check my math on the cfgs - I think this is a better arrangement and pass my various checks, but need a double check

@cwfitzgerald cwfitzgerald force-pushed the re-export-public-dependencies branch from a3dfbe3 to f47675e Compare January 15, 2024 07:25
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

cfgs all look right to me, just a few cosmetic comments
this will be super useful!

wgpu/src/backend/webgpu.rs Show resolved Hide resolved
wgpu/Cargo.toml Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/util/init.rs Show resolved Hide resolved
Copy link
Contributor

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

cfgs seem fine to me.
I think renaming the documented cfg with doc(cfg(...)) like you did is a good idea.

@cwfitzgerald
Copy link
Member Author

I wonder if it's worth documenting the feature shorthands in the lib.rs file so the user knows what to expect and we don't need to rename all the shorthands

@daxpedda
Copy link
Contributor

I wonder if it's worth documenting the feature shorthands in the lib.rs file so the user knows what to expect and we don't need to rename all the shorthands

I believe this would be an improvement in any case 👍.

@cwfitzgerald cwfitzgerald force-pushed the re-export-public-dependencies branch from f47675e to 3baede1 Compare January 16, 2024 18:50
@cwfitzgerald cwfitzgerald force-pushed the re-export-public-dependencies branch from 3baede1 to 6032442 Compare January 16, 2024 18:58
@cwfitzgerald cwfitzgerald enabled auto-merge (squash) January 16, 2024 18:58
@cwfitzgerald cwfitzgerald merged commit 2e38187 into gfx-rs:trunk Jan 16, 2024
27 checks passed
@cwfitzgerald cwfitzgerald deleted the re-export-public-dependencies branch January 16, 2024 19:25
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.

Re-Export All Public Dependencies
3 participants