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

Fix panic in examples using argh on the web #11513

Merged
merged 2 commits into from
Jan 24, 2024

Conversation

rparrett
Copy link
Contributor

Objective

Fixes #11503

Solution

Use an empty set of args on the web.

Discussion

Maybe in the future we could wrap this so that we can use query args on the web or something, but this was the minimum changeset I could think of to keep the functionality and make them not panic on the web.

@alice-i-cecile alice-i-cecile added C-Examples An addition or correction to our examples O-Web Specific to web (WASM) builds labels Jan 24, 2024
@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Jan 24, 2024
@alice-i-cecile alice-i-cecile added the C-Bug An unexpected or incorrect behavior label Jan 24, 2024
@rparrett rparrett changed the title Fix examples using argh on the web Fix panic in examples using argh on the web Jan 24, 2024
@Elabajaba
Copy link
Contributor

Imo Args::default() would be cleaner, and it should have a comment on each of them saying from_env panics on web.

@rparrett
Copy link
Contributor Author

rparrett commented Jan 24, 2024

Imo Args::default() would be cleaner

I am not sure how that would work.

edit: to expand on that, Args doesn't impl Default, and argh has its own ideas about how to specify defaults. We would need to do a manual Default impl and always keep it in sync with argh's annotations.

@mockersf
Copy link
Member

didn't saw a better way to do this in argh doc, and I think it's better than to specify defaults twice, once for argh and once for the Default trait

@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 24, 2024
@mockersf mockersf added this pull request to the merge queue Jan 24, 2024
Merged via the queue into bevyengine:main with commit bcbab18 Jan 24, 2024
23 checks passed
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
# Objective

Fixes bevyengine#11503

## Solution

Use an empty set of args on the web.

## Discussion

Maybe in the future we could wrap this so that we can use query args on
the web or something, but this was the minimum changeset I could think
of to keep the functionality and make them not panic on the web.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Bug An unexpected or incorrect behavior C-Examples An addition or correction to our examples O-Web Specific to web (WASM) builds S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some examples are broken on web due to calling argh::from_env()
4 participants