search

tompave / fun_with_flags

Feature Flags/Toggles for Elixir
https://hexdocs.pm/fun_with_flags/FunWithFlags.html
MIT License
1.08k stars 79 forks source link

Add support for SQLite #151

Closed tylerbarker closed 1 year ago

tylerbarker commented 2 years ago

Overview

Hello 👋 thanks for this library!

I'm currently working on a project using SQLite via the ecto_sqlite3 adapter. I was integrating FunWithFlags and found that only Postgres and MySQL were supported, so I thought I'd have a go at extending that. Let me know what you think.

~Note: I made some edits to ecto_test.exs as there were a couple of tests which were flaky even without these changes. My understanding is these updates (sorting for list equality) don't relate to the underlying functionality and so the tests continue to assert the same thing.~

tompave commented 2 years ago

Thank you for using the library and for opening this PR. A few comments before doing a deeper review:

  1. I would remove https://github.com/tompave/fun_with_flags/pull/151/commits/f375eb9af8a9f7a2b413e8a11e85a6b21bcf8257, which fixes the flaky tests, from the PR. I thank you for the fix and I would be happy to accept it as a separate PR.
  2. I can see a lot of extra formatting changes in some some files (trailing commas, parenthesis, etc). Please revert them. One day perhaps I'll pass this whole project through the formatter, but a feature PR is not the right place for it.
  3. I can see that you've already bumped the package version in separate files. Please revert that. The release process for this package is based on bumping all those references in an isolated commit that gets git-tagged. I'll do it when it's time. This includes the changelog. You can add a new heading Unreleased.
  4. I can see some serious refactoring of a DB migration. I'm not sure about those changes. First off, that PR was only necessary to upgrade the table in applications that were using this package before a certain change. I don't think it's really relevant for SQLite, so I would revert those changes. Even if it were necessary, if you drop and re-add a column you'll be dropping the data in the table I think, which is a no-no.
tylerbarker commented 2 years ago

Hey, thanks for the detailed feedback. I think I've addressed your comments now. I'll raise the test changes in a separate PR like you suggest.

tylerbarker commented 2 years ago

Looks like the tests are failing due to the migration being incompatible with SQLite. I wonder would it be worth re-introducing my migration changes, but configured alongside the original, so that the remove / add version only runs when System.get_env("RDBMS") is set to "sqlite3"?

tompave commented 2 years ago

Looks like the tests are failing due to the migration being incompatible with SQLite. I wonder would it be worth re-introducing my migration changes, but configured alongside the original, so that the remove / add version only runs when System.get_env("RDBMS") is set to "sqlite3"?

I see. I don't remember exactly what you were changing in the migration (I didn't have the time to do a deep review), and now I can't access it anymore because you git-amended and force-pushed, and I can't get the SHA of that first commit. (Going forward, let's just push new commits without force-pushes please; amending and force-pushing nukes the history and makes re-reviews more difficult.)

The error seems to come from the second migration, which is really just an upgrade step for v1.1.0. I remember thinking that maybe it's time to remove that second migration from master, since it's been a while and it can still be linked in the changelog. I'll do that tomorrow or next week, then you can merge master into this PR.

tylerbarker commented 1 year ago

Understood, good point on sqlite3 -> sqlite 👍🏼 I'll keep an eye out for your second migration changes, cheers.

tylerbarker commented 1 year ago

I'm using it in my side project Prototape - which is a LiveView application. I read an interesting article from Fly about the potential for SQLite in full-stack apps, so I'm giving it a go.

tylerbarker commented 1 year ago

Hey @tompave, any updates on this?

simoncocking commented 1 year ago

Just a quick note to say we're super excited by the prospect of Sqlite support in FunWithFlags - we're eyeing a move from Postgres to Sqlite in our app. Oban and FwF were the two roadblocks; Oban has since announced support. We'd love to see some movement on this PR 😉

P.S. @tylerbarker huge props for getting the ball rolling on this! 😁

tompave commented 1 year ago

Hey there, apologies for disappearing. I'll be looking at the recent activity here this week.