Skip to content
This repository was archived by the owner on Dec 6, 2022. It is now read-only.

Comments

Integrate ActiveRecord#44

Merged
jcs merged 9 commits intojcs:masterfrom
universal:active-record
Jul 30, 2018
Merged

Integrate ActiveRecord#44
jcs merged 9 commits intojcs:masterfrom
universal:active-record

Conversation

@universal
Copy link
Contributor

@universal universal commented Mar 24, 2018

see also #43 , just for simplification I copy the text I wrote there, the ticket can probably be closed:

I'm currently having a look at bringing in activerecord for the db stuff. It would open up the possibility to easily use other db systems. It would also probably simplify serializing data to and from the db in json format.

Some questions came up:

  1. The standalone-migrations gem in the current version depends on nearly everything of rails. The question is if that is something that is ok? It handles all the required rake tasks. The alternative is to just implement("copy from examples") the rake tasks.

sinatra-active-record gem provides the rake tasks and the integration with sinatra. If the last commit is not applied, the standalone-migrations gem is used. Both versions should just work fine.

  1. Migrating the current db: As it is, the current db can not be dumped into the ar-schema format since ar does not know how to handle the "STRING" column type, which doesn't even seem to be a valid sqlite column type and sqlite just seems to ignore it. So either just live with the current db and only implement changes to it using the ar-migrations or create a new one using migrations and definitions from rails and then migrate the data only.

There is a migration tool which should work fine with converting the database.

  1. Should the uuid string primary / foreign keys be used or use the default integer based keys from activerecord and just keep the uuid columns as unique ones?

uuid primary / foreign keys kept

  1. Should the manual serialization of some fields be kept or should it be adopted that the serialize :some_field helper from rails is used and a dedicated json serializer is added?

rails serialize helper with the JSON serializer used.

Current status:

  • app loads with ar
  • basic stuff like User.first works
  • recreated schema creation using ar-migrations
  • basic migration tool to migrate from old "manual" db to ar-db implemented
  • added associations
  • specs pass
  • serialization of fields for saving
  • updated readme
  • check creation / update from params
  • syncing, creation and updating of records works

Further refactorings

  • serialization of records to json for the api responses with "
  • optional: factory_bot or alike for specs: should simplify specs a lot
  • separate specs into model and request specs
  • extend test suite to include more request specs, to check example for different parameters sent from clients and proper handling of them

Open Questions

  • auto-run migrations on load?!

@universal
Copy link
Contributor Author

using janko-m/sinatra-activerecord might be the more logical choice...

@universal
Copy link
Contributor Author

universal commented Mar 25, 2018

Switching to sinatra-activerecord isn't complicated and reduces the number of gems required "from" rails. I've created a branch for that based off this one.

I've so far only run the specs, but haven't tested against the browser client. The specs are passing now. I'll manually check the browser plugin next.

If someone wants to give it a try, I highly suggest not doing it in a production env!

bundle --with migrate
# the following will migrate the development.sqlite3 file to the "ar-format"
# it will backup the original file to development.sqlite3.CURRENT_TIMESTAMP
ruby tools/migrate_to_ar.rb -e development
env RACK_ENV=development ALLOW_SIGNUPS=1 bundle exec rackup -p 4567 config.ru

@jcs jcs mentioned this pull request Mar 27, 2018
@universal
Copy link
Contributor Author

This should now be complete, some further testing would be very much appreciated! I checked it against my database and it seems to work fine and the firefox and chrome browser clients. I currently don't have a mobile device handy for testing.

Proper handling of Identies and Cards depends on #47.

@universal universal mentioned this pull request May 2, 2018
@universal
Copy link
Contributor Author

@jcs any feedback on this? If you don't want to integrate this, that is totally fine with me, but at least some comment would be nice :-) If you are busy with other stuff and can't look at it right now, I don't mind keeping it open for as long as it takes.

@jcs
Copy link
Owner

jcs commented Jul 12, 2018

If you fix the merge conflicts I can try it out. I'm not opposed to switching to AR, there is just a ton of stuff in your PRs (this one and #52) so I don't know what is done and what you're still working on.

@universal
Copy link
Contributor Author

This one is just switching to using active-record. The other one is all the additional stuff like attachments etc... I will fix the merge conflicts and ping you again when i'm done :-)

@universal universal changed the title WIP: Integrate ActiveRecord Integrate ActiveRecord Jul 29, 2018
@universal
Copy link
Contributor Author

@jcs I updated the commits. It should now merge just fine. I also added the sinatra-active-record commit to this pull-request, since I feel this is the more logical choice.

@jcs
Copy link
Owner

jcs commented Jul 30, 2018

Seems to work here with some minor changes, I'll clean those up after merging.

Thanks for all the work involved in this.

@jcs jcs merged commit 5ab123d into jcs:master Jul 30, 2018
@universal universal deleted the active-record branch August 18, 2018 09:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants