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

Update to Sequel 5.0.0 #143

Merged
merged 12 commits into from
Oct 9, 2017
Merged

Conversation

Jesterovskiy
Copy link

No description provided.

@ckoenig
Copy link

ckoenig commented Sep 29, 2017

Since Sequel 5 immediately attempts to connect to the configured database, the development DB sequel_rails_test_storage_dev needs to be created, too.

Adding the following two lines to .travis.yml will hopefully fix most of the failed tests:

- mysql -e 'create database sequel_rails_test_storage_dev;'
- psql -c 'create database sequel_rails_test_storage_dev;' -U postgres

@ckoenig
Copy link

ckoenig commented Oct 2, 2017

The sqlite3 tests are failing because SequelRails::DbConfig#build_url converts a relative path into an absolute path. Combined with the fact, that Sequel 5 is now eagerly trying to connect to a DB, the test fails.

To fix the test, it might be enough to replace the relative with an absolute path in the Rakefile:

diff --git a/Rakefile b/Rakefile
index 5d991e4..995ab3a 100644
--- a/Rakefile
+++ b/Rakefile
@@ -30,7 +30,7 @@ begin
 
     configs = {
       'postgresql' => { 'TEST_ENCODING' => 'unicode' },
-      'sqlite3'    => { 'TEST_DATABASE' => 'db/database.sqlite3' },
+      'sqlite3'    => { 'TEST_DATABASE' => File.join(File.expand_path("."), "spec/internal/db/database.sqlite3") },
     }

@Jesterovskiy
Copy link
Author

@ckoenig Thank you for your help! yesterday I seems to have folded the wrong way =)

@Jesterovskiy
Copy link
Author

@JonathanTron @JosephHalter I think PR ready for review =)

@JonathanTron
Copy link
Member

@Jesterovskiy Thanks a lot for the amazing work!

I'll take some time to review and merge in the next couple of days.

Copy link

@bgentry bgentry left a comment

Choose a reason for hiding this comment

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

LGTM aside from maybe wanting to bump the latest ruby to 2.4.2

- 2.4.0
- 2.2.7
- 2.3.4
- 2.4.1
Copy link

Choose a reason for hiding this comment

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

maybe this should be 2.4.2 ?

gemfile: ci/rails-4.0.gemfile
- rvm: 2.4.0
- rvm: 2.4.1
Copy link

Choose a reason for hiding this comment

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

probably also 2.4.2 on these


s.add_dependency 'activemodel', '>= 4.0.0'
s.add_dependency 'railties', '>= 4.0.0'
s.add_dependency 'actionpack', '>= 4.0.0'
s.add_dependency 'sequel', ['>= 3.28', '< 5.0']
s.add_dependency 'sequel', ['>= 3.28', '<= 5.0']
Copy link

@bgentry bgentry Oct 9, 2017

Choose a reason for hiding this comment

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

I don't think we want to lock this in to <= 5.0. Sequel is already on 5.1.0. Possibly < 6.0 instead?

Copy link

Choose a reason for hiding this comment

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

@Jesterovskiy I opened a PR to fix this specific issue Jesterovskiy#1

@JonathanTron
Copy link
Member

Thanks @Jesterovskiy for the good work, I'll merge the request and change the dependencies to deps to work with Sequel < 6.0 as @bgentry suggested.

@JonathanTron JonathanTron merged commit aae5cad into TalentBox:master Oct 9, 2017
@Jesterovskiy
Copy link
Author

@JonathanTron Thank you! Now you can release this, right?

@Jesterovskiy Jesterovskiy deleted the update-gemspec branch October 23, 2017 07:58
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.

4 participants