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

split floor into floor_common, floor, floor_ffi to be able to use floor without flutter. #638

Merged
merged 27 commits into from
May 6, 2024

Conversation

rivasdiaz
Copy link
Contributor

closes #637

  • splits floor in floor_common, floor_ffi and floor.
  • floor_common and floor_ffi do not depend on flutter or sqflite (only sqflite_common and sqflite_common_ffi)
  • floor depends on flutter, sqflite and sqflite_common_ffi, re-exports all floor_common

@vitusortner
Copy link
Collaborator

thanks for implementing this improvement! i'll align our ci setup to these changes and eventually, i'll report back with suggested improvements.

@rivasdiaz
Copy link
Contributor Author

Thanks for considering my MR!

I've been successfully using my branch in a small project in which I wanted to write a dart only CLI to use on desktop.

One small issue I see is the generated code depends on sqfliteDatabaseFactory which needs to be initialized differently if using floor (flutter) or floor_ffi (dart only). The implication is the code declaring the database has to import one or the other. As a result, the database class cannot be added to a common package imported by both a flutter app or a dart only cli app. This doesn't seem to be a big issue IMHO (an abstract base class gets most of the job done), but just something for you to be aware.

If you have any concerns, I'll be happy to update this MR. Or feel free to use my MR as you see fit in case you want to use it as a basis to do your own MR.

Thanks!

@alimcomp
Copy link

@vitusortner Could you please . merge this pull request

stephanmantel
stephanmantel previously approved these changes Mar 25, 2024
Copy link

codecov bot commented Mar 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.31%. Comparing base (4146037) to head (28d05d9).
Report is 1 commits behind head on develop.

❗ Current head 28d05d9 differs from pull request most recent head 435813a. Consider uploading reports for the commit 435813a to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #638      +/-   ##
===========================================
+ Coverage    91.51%   94.31%   +2.79%     
===========================================
  Files           11       10       -1     
  Lines          224      211      -13     
===========================================
- Hits           205      199       -6     
+ Misses          19       12       -7     
Flag Coverage Δ
floor 94.31% <ø> (+2.79%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

stephanmantel
stephanmantel previously approved these changes Mar 25, 2024
@stephanmantel
Copy link
Contributor

I see this PR checks in the generated code now. We did not have this before and they were git ignored, but only under /floor. Now that all this lives under floor_common, the gitignore must be updated. @hendrikvdkaaden Can you remove the generated files from git and after that update the gitignore file?

@stephanmantel stephanmantel merged commit e27f92c into pinchbv:develop May 6, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

enhancement request: support dart without flutter
5 participants