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(macro): handle patterns in inline_props component arguments #720

Closed
wants to merge 1 commit into from

Conversation

KekmaTime
Copy link

closes #718

The fix removes pattern modifiers when generating the props struct, ensuring valid syntax
while preserving intended mutability semantics within the component function.

Copy link

codecov bot commented Oct 15, 2024

Codecov Report

Attention: Patch coverage is 85.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 64.01%. Comparing base (ad04ae9) to head (d6c50c9).
Report is 35 commits behind head on main.

Files with missing lines Patch % Lines
packages/sycamore-macro/src/component.rs 85.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #720      +/-   ##
==========================================
+ Coverage   63.97%   64.01%   +0.04%     
==========================================
  Files          52       52              
  Lines        7072     7087      +15     
==========================================
+ Hits         4524     4537      +13     
- Misses       2548     2550       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@lukechu10 lukechu10 left a comment

Choose a reason for hiding this comment

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

Ah I've just realized that supporting arbitrary patterns for inline props means that we no longer know what the prop name is.

Maybe we can only support mut/ref patterns and {xyz} @ ... patterns so that we always have an identifier we can use (where {xyz} is any binding, so we need to get rid of mut/ref again).

Comment on lines -268 to +288
let phantom_ident = format_ident!("__phantom{i}");
let phantom_ident = format_ident!("__phantom{}", i);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why you changed this

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure why you changed this

i think that is the wrong way of formatting. format_ident!("__phantom{i}") treats {i} as a literal, so the identifier becomes __phantom{i} exactly, without replacing i with its value.

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure the {i} works because it generates the right code. Also format_ident! uses std::format! under the hood and the new format!() syntax is supported since Rust 1.58.

Copy link
Author

@KekmaTime KekmaTime Oct 15, 2024

Choose a reason for hiding this comment

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

I'm pretty sure the {i} works because it generates the right code. Also format_ident! uses std::format! under the hood and the new format!() syntax is supported since Rust 1.58.

im sorry i didnt know that 😓 ill fix it right away

@lukechu10
Copy link
Member

Also it would be great if you could add some tests to packages/sycamore-macro/tests/component/inline-props-pass.rs

@KekmaTime
Copy link
Author

Also it would be great if you could add some tests to packages/sycamore-macro/tests/component/inline-props-pass.rs

sure ill get to it right away 👍🏽

@lukechu10
Copy link
Member

Hi I'm wondering if you maybe forgot to push your changes to the remote or if you're still working on this. Either way is fine!

@lukechu10 lukechu10 added the C-bug Category: bug, something isn't working label Oct 19, 2024
@lukechu10 lukechu10 closed this Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug, something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inline props does not properly support patterns
2 participants