[Fix #495] Default rails console strategy configuration #780

Vitalina-Vakulchyk · 2021-04-01T09:25:26Z

Remove default setting of :urgent strategy for console if there is another current strategy.

The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
Wrote good commit messages.
Commit message starts with [Fix #issue-number] (if the related issue exists).
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
~~Updated tests.~~
Added an entry to the changelog if the new code introduces user-observable changes. See changelog entry format for details.

dalthon

🚀 LGTM!

Just left a few comments that you can change if you like and one detail that might cause some issues with Ruby 2.5, but I will not block this PR.

CHANGELOG.md

lib/chewy/config.rb

lib/chewy/type/witchcraft.rb

barthez · 2021-04-01T13:46:06Z

Will it close #495?

barthez · 2021-04-01T13:49:11Z

.rubocop.yml

@@ -2,6 +2,7 @@ inherit_from: .rubocop_todo.yml

 AllCops:
  NewCops: enable
+  TargetRubyVersion: 2.7


As Dalton mentioned, we aim for 2.5. Please set it up here as well.

We check only 2.6, 2.7 & 3.0. Let's aim for 2.6 and fix the README!

Vitalina-Vakulchyk · 2021-04-01T13:54:36Z

@barthez Yes, this PR will solve this issue #495
I am going to leave the accompanying comment there after merging.

dalthon · 2021-04-01T13:58:54Z

@Vitalina-Vakulchyk I guess @barthez asked about solving that issue because then you can follow that check on PR description: Commit message starts with [Fix #issue-number] (if the related issue exists). and rename your PR description accordingly.
It helps us to keep track of PRs and Issues and I believe it automatically closes the issue once this PR is merged

Vitalina-Vakulchyk · 2021-04-01T14:02:53Z

Thank you! I have changed the PR name.

…ot other

rabotyaga

Please add a test

CHANGELOG.md

lib/chewy/config.rb

rabotyaga

What about tests?

rabotyaga · 2021-04-02T14:46:37Z

lib/chewy/railtie.rb

@@ -37,7 +37,8 @@ def migrate(*args)
      if app.sandbox?
        Chewy.strategy(:bypass)
      else
-        Chewy.strategy(:urgent)
+        strategy = Chewy.console_strategy || :urgent
+        Chewy.strategy(strategy)


Let's initialize it properly in https://github.com/toptal/chewy/pull/780/files#diff-542f2b8cbb20f056db635819b1e65c367d4e8fdb9b7e6ae000ecda0c107f1127R52-R64

lib/chewy/config.rb

CHANGELOG.md

dalthon · 2021-04-02T15:22:31Z

@rabotyaga how do you think this should be tested?
Adding rails as a test dependency just to invoke Rails::Console.new like is tested at Rails itself just to capture its IO?
Do you think that it worth? Or have other idea about how to do it?

rabotyaga · 2021-04-02T16:27:05Z

@rabotyaga how do you think this should be tested?
Adding rails as a test dependency just to invoke Rails::Console.new like is tested at Rails itself just to capture its IO?
Do you think that it worth? Or have other idea about how to do it?

We could add just a couple of lines to https://github.com/toptal/chewy/blob/master/spec/chewy/config_spec.rb

…NGELOG

Vitalina-Vakulchyk · 2021-04-05T09:45:52Z

@rabotyaga I added the test for default console_strategy value.

rabotyaga

Can we test setter also?

Vitalina-Vakulchyk · 2021-04-05T11:16:40Z

@rabotyaga I have added setter spec.

rabotyaga · 2021-04-05T11:39:15Z

spec/chewy/config_spec.rb

+
+  describe '.console_strategy' do
+    context 'sets .console_strategy' do
+      let(:strategy) { :atomic }


Should we use the after hook to prevent the leak of the state?

@rabotyaga Thank you for noticing this.
I added the after hook.

…#780)

Vitalina-Vakulchyk force-pushed the defaul-rails-console-strategy-configuration branch from 87ef907 to f71f39b Compare April 1, 2021 13:24

dalthon approved these changes Apr 1, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

lib/chewy/config.rb Outdated Show resolved Hide resolved

lib/chewy/type/witchcraft.rb Outdated Show resolved Hide resolved

barthez approved these changes Apr 1, 2021

View reviewed changes

Vitalina-Vakulchyk changed the title ~~Default rails console strategy configuration~~ [Fix #495] Default rails console strategy configuration Apr 1, 2021

Vitalina-Vakulchyk force-pushed the defaul-rails-console-strategy-configuration branch from 6cb8572 to 6eb947c Compare April 2, 2021 13:26

rabotyaga force-pushed the master branch from 1b54be4 to fe5e411 Compare April 2, 2021 13:32

Vitalina-Vakulchyk added 6 commits April 2, 2021 16:50

Ignore empty blank options on fields (#778)

6f31d43

Update console settings in realtie.rb

54ee6e5

Update console strategy setting when there is no any strategy

526bb12

Set :urgent as default console strategy just in case where there is n…

4b4f958

…ot other

Add rdoc documentation for :console_strategy Chewy option

99d4fe9

Expand console strategies setting explanation in /lib/chewy/config.rb

2dbebb4

Vitalina-Vakulchyk force-pushed the defaul-rails-console-strategy-configuration branch from 6eb947c to 2dbebb4 Compare April 2, 2021 13:52

Vitalina-Vakulchyk requested a review from rabotyaga April 2, 2021 13:58

rabotyaga suggested changes Apr 2, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

lib/chewy/config.rb Outdated Show resolved Hide resolved

Vitalina-Vakulchyk added 2 commits April 2, 2021 17:33

Update lib/chewy/config.rb docs order

5639fff

Update changelog new lines

b39e129

Vitalina-Vakulchyk requested a review from rabotyaga April 2, 2021 14:41

rabotyaga suggested changes Apr 2, 2021

View reviewed changes

Vitalina-Vakulchyk added 4 commits April 5, 2021 12:36

Initialize Chewy.console_strategy in config.rb

a2c73cf

Add spec for Chewy.console_strategy

389c87d

Update Chewy.console_strategy doc in /lib/chewy/config.rb

03e8b72

Replace link with PR to link with issue about console_strategy in CHA…

5a7420b

…NGELOG

Vitalina-Vakulchyk added 2 commits April 5, 2021 12:51

Remove variable for console_strategy setting in /lib/chewy/railtie.rb

bd71237

Remove new line in /lib/chewy/railtie.rb

bb2814e

rabotyaga reviewed Apr 5, 2021

View reviewed changes

Add spec for Chewy.console_strategy setter

1696d50

rabotyaga reviewed Apr 5, 2021

View reviewed changes

Add after hook to prevend state leak in /lib/chewy/config.rb

9502e9f

rabotyaga approved these changes Apr 5, 2021

View reviewed changes

Vitalina-Vakulchyk merged commit 86ee881 into master Apr 5, 2021

Vitalina-Vakulchyk deleted the defaul-rails-console-strategy-configuration branch April 5, 2021 12:25

cyucelen pushed a commit to cyucelen/chewy that referenced this pull request Jan 28, 2023

[Fix toptal#495] Default rails console strategy configuration (toptal…

81d9045

…#780)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix #495] Default rails console strategy configuration #780

[Fix #495] Default rails console strategy configuration #780

Vitalina-Vakulchyk commented Apr 1, 2021 •

edited

Loading

dalthon left a comment

barthez commented Apr 1, 2021

barthez Apr 1, 2021

rabotyaga Apr 1, 2021

Vitalina-Vakulchyk commented Apr 1, 2021

dalthon commented Apr 1, 2021

Vitalina-Vakulchyk commented Apr 1, 2021

rabotyaga left a comment

rabotyaga left a comment

rabotyaga Apr 2, 2021

dalthon commented Apr 2, 2021

rabotyaga commented Apr 2, 2021

Vitalina-Vakulchyk commented Apr 5, 2021 •

edited

Loading

rabotyaga left a comment

Vitalina-Vakulchyk commented Apr 5, 2021

rabotyaga Apr 5, 2021

Vitalina-Vakulchyk Apr 5, 2021

[Fix #495] Default rails console strategy configuration #780

[Fix #495] Default rails console strategy configuration #780

Conversation

Vitalina-Vakulchyk commented Apr 1, 2021 • edited Loading

dalthon left a comment

Choose a reason for hiding this comment

barthez commented Apr 1, 2021

barthez Apr 1, 2021

Choose a reason for hiding this comment

rabotyaga Apr 1, 2021

Choose a reason for hiding this comment

Vitalina-Vakulchyk commented Apr 1, 2021

dalthon commented Apr 1, 2021

Vitalina-Vakulchyk commented Apr 1, 2021

rabotyaga left a comment

Choose a reason for hiding this comment

rabotyaga left a comment

Choose a reason for hiding this comment

rabotyaga Apr 2, 2021

Choose a reason for hiding this comment

dalthon commented Apr 2, 2021

rabotyaga commented Apr 2, 2021

Vitalina-Vakulchyk commented Apr 5, 2021 • edited Loading

rabotyaga left a comment

Choose a reason for hiding this comment

Vitalina-Vakulchyk commented Apr 5, 2021

rabotyaga Apr 5, 2021

Choose a reason for hiding this comment

Vitalina-Vakulchyk Apr 5, 2021

Choose a reason for hiding this comment

Vitalina-Vakulchyk commented Apr 1, 2021 •

edited

Loading

Vitalina-Vakulchyk commented Apr 5, 2021 •

edited

Loading