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

Implement filter helper and use it to suppress reactive variables for non-widget fields #128

Merged
merged 14 commits into from
Jan 29, 2024

Conversation

pdaoust
Copy link
Collaborator

@pdaoust pdaoust commented Sep 28, 2023

#126 solved the problem of EditFoo.svelte templates creating trailing commas in the reactive variable declarations; e.g., for an entry type Foo with fields visible_name and hidden_birthdate, the template would generate this line:

$: visible_name, ;

But it was an ugly solution. This introduces a better solution via a new filter helper for Handlebars. You pass it a value containing a list of items and a predicate, and it gives back a list of filtered items. Then you can filter out non-widget fields and use HBS' last helper to suppress the final comma. (I'm sure it has other uses too.)

It just renders a very tiny Handlebars template that uses {{#if}} to check the predicate against each item. This lets you use Handlebars' full functionality in the predicate.

@pdaoust
Copy link
Collaborator Author

pdaoust commented Sep 29, 2023

@robbiecarlton the biggest thing I'm worried about with this PR is that I kinda just cargo-culted the ownership/borrowing/consuming thing until rustc shut up. Not sure if what I did was good practice. And I also don't know about the iterator chain I used for filtering the items -- feels like there ought to be some more elegant way of filtering and failing on the first error, but I tried a few ideas and then gave up. Currently it throws out runtime handlebars errors, but it does test the template once with the first item in the list to make sure the passed-in filter predicate actually works. (The way it works is that it just creates a tiny template, {{#if <condition>}}true{{else}}false{{/if}} and then passes each item to that template in turn.)

@ThetaSinner
Copy link
Member

ThetaSinner commented Nov 2, 2023

@pdaoust A few things I'd like to do to get this merged

  • Please can the PR target the develop branch and I'll take care of merging it to release series branches
  • Could you please give me some steps to reproduce the problem you're fixing so I can do my own manual testing?
  • Are you up for adding some unit tests where appropriate for this? I'm not sure what's already here so it might not be super straight forward. I'm happy to either work on this with you or take it on myself based on some manual testing

@ThetaSinner
Copy link
Member

I'm going to go ahead and re-target this to develop because the current target branch is going to be renamed

@ThetaSinner ThetaSinner added ShouldBackport/0.1 This change should be backported to develop-0.1 ShouldBackport/0.2 This change should be backported to develop-0.2 labels Nov 3, 2023
@ThetaSinner ThetaSinner changed the base branch from holochain-0.1 to develop November 3, 2023 16:49
@ThetaSinner
Copy link
Member

Okay, this is now ready for review and can be merged when ready

Copy link
Collaborator

@c12i c12i left a comment

Choose a reason for hiding this comment

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

Better solution than what is proposed in #130 since we skip adding a widget_fields field to the EntryDefinition struct. I lean more towards this option as well

However, @ThetaSinner is right, some unit tests for the filter helper will definitely be needed, with cases covering usage of the includeZero optional parameter

@pdaoust kindly let me know if you are able to resolve the merge conflicts in this PR too

src/templates.rs Outdated Show resolved Hide resolved
src/templates.rs Outdated Show resolved Hide resolved
src/templates.rs Outdated Show resolved Hide resolved
src/templates.rs Outdated Show resolved Hide resolved
@pdaoust
Copy link
Collaborator Author

pdaoust commented Jan 19, 2024

@ThetaSinner really sorry I missed all your messages from November; I don't have a good way of getting notifications from GitHub. @c12i thanks for the review; I'm really eager to get feedback on the aesthetics/idiomatic-ness of my code. Rust is very new to me, and I'd prefer iterators (coming from C# and JS) but don't always know how to do them in Rust.

I'll merge from develop and take a look at all the review stuff; thanks all!

@pdaoust
Copy link
Collaborator Author

pdaoust commented Jan 22, 2024

Okay, I've tidied up all the code per your feedback @c12i and I've also merged in develop -- I noticed that the helpers code was rearranged, so I moved the new filter helper into a similar structure (it gets its own file, figure it was sufficiently complex).

I'm pretty new to writing Rust, but I think I could come up with some tests that at the very least test the initial problem that this thing was trying to fix. How far do you think I should go -- wondering if it makes sense to test all possible falsey HBS values and a few representative truthy values, or if that's just overkill.

@c12i
Copy link
Collaborator

c12i commented Jan 23, 2024

I noticed that the helpers code was rearranged, so I moved the new filter helper into a similar structure (it gets its own file, figure it was sufficiently complex).

Ah yes, should have mentioned this earlier, glad you made the change

How far do you think I should go -- wondering if it makes sense to test all possible falsey HBS values and a few representative truthy values, or if that's just overkill.

No worries @pdaoust, we can test the happy path cases for now

src/templates/helpers/filter.rs Outdated Show resolved Hide resolved
Co-authored-by: Collins Muriuki <[email protected]>
@pdaoust
Copy link
Collaborator Author

pdaoust commented Jan 25, 2024

@c12i could you review again? I simplified some code -- realised that a couple suggestions above tackled one problem in two different ways and cancelled each other out :) I also wrote a few basic tests and discovered/fixed a bug.

@c12i
Copy link
Collaborator

c12i commented Jan 26, 2024

realised that a couple suggestions above tackled one problem in two different ways and cancelled each other out

thanks for the changes @pdaoust, the implementation is much simpler now

@c12i c12i merged commit 3302bdb into develop Jan 29, 2024
2 checks passed
@c12i c12i deleted the 2023-09-28-filter-helper branch January 29, 2024 17:34
c12i added a commit to c12i/scaffolding that referenced this pull request Feb 8, 2024
Implement `filter` helper and use it to suppress reactive variables for non-widget fields
c12i added a commit to c12i/scaffolding that referenced this pull request Feb 8, 2024
@c12i c12i removed ShouldBackport/0.1 This change should be backported to develop-0.1 ShouldBackport/0.2 This change should be backported to develop-0.2 labels Feb 8, 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.

3 participants