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

Sort "Recent entries" for water #6

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

janraasch
Copy link

Solves #2


context

Hi @joemasilotti, nice to meet you 🤝. I came across this repo by way of your latest newsletter. I am in the process of writing a Rails x Tailwind x Turbo-app myself, so I decided to dig into the source code here a little bit. The help wanted-issues are great to solve some issues while looking around the code base.

solution

The sorting happens on the ruby-level (i.e. using sort_by). This way we do not have to cross the #amount-abstraction level implemented by measured-rails. Performance-wise - I think - it's okay, since we'll have to load the three WaterEntrys from the db for rendering the view anyway, so we might as well do the sorting after loading everything from the db.

I added two clean up-type commits on top.

Let me know what you think.

Kind regards from Germany 🇩🇪,
Jan

@joemasilotti
Copy link
Owner

Thanks for the PR! And I'm excited to see what you build with Turbo Native.

Unfortunately adding this sort doesn't change the underlying problem - the query is still returning duplicate entries. I just pushed a failing test to show what I mean. Let me know if that doesn't make sense!

@janraasch
Copy link
Author

Hi @joemasilotti, having a failing test case for #7 is great 👍.

I pushed (what I proposed on #7) 7c85026 which fixes the test case:

bin/rails test test/models/water_entry_tests.rb
Running 1 tests in a single process (parallelization threshold is 50)
Run options: --seed 63187

# Running:

.

Finished in 0.073761s, 13.5573 runs/s, 13.5573 assertions/s.
1 runs, 1 assertions, 0 failures, 0 errors, 0 skips

However, if I understand your comment here correctly, this is not what you want to have.

Could you add another failing test to detail the expected behaviour further 🙏?

Test driven-feature-specification 😇 😉 ☺️.

Have an amazing Pre-Christmas weekend 🎄👋,
Jan

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.

2 participants