Register inline sequence to allow for rewinding #1164
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
houndci-bot
reviewed
Aug 4, 2018
spec/factory_bot/definition_spec.rb
Outdated
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
Layout/TrailingBlankLines: 1 trailing blank lines detected.
| proxy.sequence(:sequence, 1, &sequence_block) | ||
|
|
||
| expect(FactoryBot::Sequence).to have_received(:new) | ||
| .with("__factory_sequence__", 1, &sequence_block) |
There was a problem hiding this comment.
Layout/DotPosition: Place the . on the previous line, together with the method call receiver.
| proxy.sequence(:sequence, "C") | ||
|
|
||
| expect(FactoryBot::Sequence).to have_received(:new) | ||
| .with("__factory_sequence__", "C") |
There was a problem hiding this comment.
Layout/DotPosition: Place the . on the previous line, together with the method call receiver.
| proxy.sequence(:sequence) | ||
|
|
||
| expect(FactoryBot::Sequence).to have_received(:new) | ||
| .with("__factory_sequence__") |
There was a problem hiding this comment.
Layout/DotPosition: Place the . on the previous line, together with the method call receiver.
| expect(email).to eq "[email protected]" | ||
| end | ||
|
|
||
| it "still allows global sequences prefixed with a factory name", :aggregate_failures do |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [89/80]
| expect(user.email).to eq "[email protected]" | ||
| end | ||
|
|
||
| it "does not collide with globally registered factories", :aggregate_failures do |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [82/80]
This was originally opened as #1078, but this addresses the review comments on that PR. By registering the inline sequences, we allow them to get rewound with `FactoryBot.rewind_sequences`. We register them with `__#{factory_name}_#{sequence_name}__` to avoid conflicting with any reasonably named global sequences, and to hint that we should not be generating values from these sequences directly. Co-authored-by: Damian Le Nouaille <[email protected]> Co-authored-by: Damian Galarza <[email protected]>
composerinteralia
force-pushed
the
rewind-local-sequences
branch
from
ca51b68 to
4ec7283
Compare
mike-burns
approved these changes
Aug 10, 2018
composerinteralia
added a commit
that referenced
this pull request
Feb 5, 2019
Fixes #1257 When sequence rewinding was first introduced in #1078 it only applied to globally defined sequences. To get rewinding to work for inline sequences as well we registered them "privately" in the global registry in #1164. Unfortunately in #1164 we did not take inline sequences inside traits into consideration. Since trait names are not unique, it is possibly to get a `FactoryBot::DuplicateDefinitionError` when defining two sequences that have the same name in two traits that have the same name. This PR abandons the idea of "privately" registering inline sequences, and instead keeps a separate list of all the inline sequences just for the purpose of rewinding them. The downside here is that we are adding another public method to FactoryBot. But I think it is similar enough to the other `register_` methods, which are technically public but are left undocumented and not really treated as part of the FactoryBot API.
composerinteralia
added a commit
that referenced
this pull request
Feb 5, 2019
Fixes #1257 When sequence rewinding was first introduced in #1078 it only applied to globally defined sequences. To get rewinding to work for inline sequences as well we registered them "privately" in the global registry in #1164. Unfortunately in #1164 we did not take inline sequences inside traits into consideration. Since trait names are not unique, it is possibly to get a `FactoryBot::DuplicateDefinitionError` when defining two sequences that have the same name in two traits that have the same name. This PR abandons the idea of "privately" registering inline sequences, and instead keeps a separate list of all the inline sequences just for the purpose of rewinding them. The downside here is that we are adding another public method to FactoryBot. But I think it is similar enough to the other `register_` methods, which are technically public but are left undocumented and not really treated as part of the FactoryBot API.
composerinteralia
added a commit
that referenced
this pull request
Feb 13, 2019
Fixes #1257 When sequence rewinding was first introduced in #1078 it only applied to globally defined sequences. To get rewinding to work for inline sequences as well we registered them "privately" in the global registry in #1164. Unfortunately in #1164 we did not take inline sequences inside traits into consideration. Since trait names are not unique, it is possibly to get a `FactoryBot::DuplicateDefinitionError` when defining two sequences that have the same name in two traits that have the same name. This PR abandons the idea of "privately" registering inline sequences, and instead keeps a separate list of all the inline sequences just for the purpose of rewinding them.
composerinteralia
added a commit
that referenced
this pull request
Feb 15, 2019
Fixes #1257 When sequence rewinding was first introduced in #1078 it only applied to globally defined sequences. To get rewinding to work for inline sequences as well we registered them "privately" in the global registry in #1164. Unfortunately in #1164 we did not take inline sequences inside traits into consideration. Since trait names are not unique, it is possibly to get a `FactoryBot::DuplicateDefinitionError` when defining two sequences that have the same name in two traits that have the same name. This PR abandons the idea of "privately" registering inline sequences, and instead keeps a separate list of all the inline sequences just for the purpose of rewinding them.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was originally opened as #1078, but this addresses the review
comments on that PR.
By registering the inline sequences, we allow them to get rewound with
FactoryBot.rewind_sequences. We register them with__#{factory_name}_#{sequence_name}__to avoid conflicting with anyreasonably named global sequences, and to hint that we should not be
generating values from these sequences directly.
Co-authored-by: Damian Le Nouaille [email protected]
Co-authored-by: Damian Galarza [email protected]