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: make directive .into() calls consistent #2249

Merged
merged 2 commits into from
Feb 5, 2024

Conversation

blorbb
Copy link
Contributor

@blorbb blorbb commented Feb 1, 2024

fixes #2248

added to the directive examples as well.

@blorbb blorbb marked this pull request as draft February 1, 2024 23:23
@blorbb
Copy link
Contributor Author

blorbb commented Feb 1, 2024

Actually, there might be a way to call Default on the parameter if no argument is given to use:, instead of requiring From<()>.
However, this would mean that users might forget to provide an argument and it unknowingly falls back to default. Could be a problem if there is no sensible default for the directive function.
@gbj do you think this is a good idea, or should we just stick to requiring From<()>? Using From instead means that directive authors can't provide both a blanket impl and a 'default', but users could still explicitly use a default with use:dir=Default::default()
Using default might be a breaking change? I don't think calling ().into() is

(Also, not sure why the CI failed above)

@benwis
Copy link
Contributor

benwis commented Feb 2, 2024

Can you run cargo clippy and/or cargo fmt on your repo? The tests are failing because of that @blorbb

@blorbb
Copy link
Contributor Author

blorbb commented Feb 2, 2024

Can you run cargo clippy and/or cargo fmt on your repo? The tests are failing because of that @blorbb

ran both, the only warning I got is about auto traits being unstable from clippy.

@benwis
Copy link
Contributor

benwis commented Feb 2, 2024

Nightly clippy is causing pain, no worries

@gbj
Copy link
Collaborator

gbj commented Feb 4, 2024

Thanks, this looks good to me for now. I agree that Default makes more sense than ().into(). It would be a good breaking change to make in the future, as it seems like a more idiomatic way to handle the optional case (and is in line with what we do with optional component props).

@gbj
Copy link
Collaborator

gbj commented Feb 4, 2024

(@blorbb You have it marked as draft, I think because of that question — To me this seems ready to merge, unless you're waiting on anything else. Is it ready to go?)

@blorbb
Copy link
Contributor Author

blorbb commented Feb 5, 2024

(@blorbb You have it marked as draft, I think because of that question — To me this seems ready to merge, unless you're waiting on anything else. Is it ready to go?)

yep was just marked draft to decide between ().into() vs Default - makes sense that it can be changed later, I'll reopen it now. Thanks!

@blorbb blorbb marked this pull request as ready for review February 5, 2024 08:02
@gbj gbj merged commit 38bf739 into leptos-rs:main Feb 5, 2024
44 of 59 checks passed
gbj pushed a commit that referenced this pull request Feb 8, 2024
videobitva pushed a commit to videobitva/leptos that referenced this pull request Feb 17, 2024
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.

inconsistent directive behaviour
3 participants