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

Add serialize, fromstr and wasm-bindgen features #592

Merged
merged 5 commits into from Oct 9, 2022

Conversation

RubixDev
Copy link
Contributor

@RubixDev RubixDev commented Sep 26, 2022

While working on creating a dprint plugin for integrating StyLua (dprint-plugin-stylua), I found few things were missing in StyLua to make the integration easier:

  1. A serde::Serialize impl for the config types
  2. A FromStr impl for the config enum types (using strum::EnumString)
  3. Disabling the wasm-bindgen crate for correct compilation

I turned these three things into features and left the defaults just like they were before.

@codecov
Copy link

codecov bot commented Sep 26, 2022

Codecov Report

Base: 97.65% // Head: 97.65% // No change to project coverage 👍

Coverage data is based on head (3edb4e4) compared to base (4ad48d0).
Patch has no changes to coverable lines.

❗ Current head 3edb4e4 differs from pull request most recent head 61f98fc. Consider uploading reports for the commit 61f98fc to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #592   +/-   ##
=======================================
  Coverage   97.65%   97.65%           
=======================================
  Files          14       14           
  Lines        5576     5576           
=======================================
  Hits         5445     5445           
  Misses        131      131           
Impacted Files Coverage Δ
src/lib.rs 96.29% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JohnnyMorganz
Copy link
Owner

JohnnyMorganz commented Sep 26, 2022

We currently run --all-features in our release CI job. We probably need to change that to explicitly only select --features lua52,lua53,lua54,luau.

Similarly wasm-bindgen should be enabled in the wasm CI job

@RubixDev
Copy link
Contributor Author

RubixDev commented Sep 26, 2022

wasm-bindgen is part of the default features, as it only has any effect for wasm32 targets anyways. I will change the workflow file to specifically list the luaXX features.

Also would you recommend enabling all the luaXX features for the dprint plugin wasm binary? As far as I understand it adds support for those Lua versions while still supporting 5.1?

@JohnnyMorganz
Copy link
Owner

Also would you recommend enabling all the luaXX features for the dprint plugin wasm binary? As far as I understand it adds support for those Lua versions while still supporting 5.1?

For simplicitly, this is what we do for our release assets, and its also what is done in the packaging manifests for stuff like homebrew.

For the most part, this is fine. But note that there can be conflicts between the different Lua versions. The main things I know of is:

Ideally we would select the syntax we are using through some sort of configuration at runtime, but currently the features are behind compile time flags so we can't do this.

If possible, my recommendation would be to produce wasm binaries for each different syntax version, then using config select the correct one to use. This comes with disadvantages though (you now need to package 5x the wasm binary). Having all features enabled should typically be fine for everyday use.

@RubixDev
Copy link
Contributor Author

Ok thanks, I think I'll just enable all Lua features then and maybe make it configurable in the future.

Copy link
Owner

@JohnnyMorganz JohnnyMorganz left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, looks good to me, thanks!

I wonder if we even need separate features for serialize and fromstr, since they are small enough. For now I'll just leave it as-is though

@JohnnyMorganz JohnnyMorganz merged commit ffec454 into JohnnyMorganz:main Oct 9, 2022
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.

None yet

2 participants