Skip to content

Don't clone when with_fallback is called#14

Merged
k0kubun merged 4 commits intocookpad:masterfrom
pocke:do-not-clone-with-fallback
Aug 24, 2016
Merged

Don't clone when with_fallback is called#14
k0kubun merged 4 commits intocookpad:masterfrom
pocke:do-not-clone-with-fallback

Conversation

@pocke
Copy link
Copy Markdown
Contributor

@pocke pocke commented Aug 24, 2016

Currently, with_fallback behavior is complexity. This cause is cloning in with_fallback method.
This change is required from #13 .

@k0kubun
Copy link
Copy Markdown
Contributor

k0kubun commented Aug 24, 2016

Tests are broken. Do you intend to change that behavior?

@pocke
Copy link
Copy Markdown
Contributor Author

pocke commented Aug 24, 2016

Yes.

Currently behavior

c = Command.new do
  xxx
end

c_fallback = c.with_fallback do
  xxx
end

c == c_fallback # => false

Changed behavior

c = Command.new do
  xxx
end

c_fallback = c.with_fallback do
  xxx
end

c == c_fallback # => true

So, the following test is broken. https://github.com/cookpad/expeditor/blob/master/spec/expeditor/command_functions_spec.rb#L179-L192
Because it depends the difference c and c_fallback.

However, in real, I think the case doesn't exist.

The test is broken by the change of PR.
@k0kubun
Copy link
Copy Markdown
Contributor

k0kubun commented Aug 24, 2016

So with_fallback is changed to be destructive, right? In my opinion, with_fallback is reasonable to be non-destructive. If the method's behavior is changed to be destructive, I think its name should be changed to set_fallback or something.

@pocke
Copy link
Copy Markdown
Contributor Author

pocke commented Aug 24, 2016

So with_fallback is changed to be destructive, right?

yes

I think its name should be changed to set_fallback or something.

👍
I'll deprecate wtih_fallback and add set_fallback. The behavior between the two methods are same. Is it ok?

@k0kubun
Copy link
Copy Markdown
Contributor

k0kubun commented Aug 24, 2016

The behavior between the two methods are same. Is it ok?

It means that it breaks backward compatibility. If you update version properly (0.5.0, since it's < 1.0), I think it'd be acceptable.

@pocke
Copy link
Copy Markdown
Contributor Author

pocke commented Aug 24, 2016

I've deprecated the method. And I've added an entry of this PR into CHANGELOG.md .
I think we can release the change with other changes( #13 and any refactor ).

@k0kubun
Copy link
Copy Markdown
Contributor

k0kubun commented Aug 24, 2016

LGTM

1 similar comment
@adorechic
Copy link
Copy Markdown

LGTM

@k0kubun k0kubun merged commit 7bb7c85 into cookpad:master Aug 24, 2016
@pocke pocke deleted the do-not-clone-with-fallback branch August 24, 2016 08:24
@pocke pocke mentioned this pull request Aug 29, 2016
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.

3 participants