Don't clear non-SuckerPunch actors from registry #113
Merged
Conversation
This file contains 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
| @@ -39,7 +39,8 @@ def registered? | |||
| end | |||
|
|
|||
| def name | |||
| klass.to_s.underscore.to_sym | |||
| klass_name = klass.to_s.underscore | |||
| "#{SuckerPunch::NAME_PREFIX}_#{klass_name}".to_sym | |||
There was a problem hiding this comment.
Is this the right place to be doing this? Does the user need to know that SuckerPunch job names are being given a prefix in the Celluloid registry?
There was a problem hiding this comment.
I think this will be fine. I'd like to use SuckerPunch::Queue as the way to get the queue, so I don't think it matters for the user to know anything about how they're registered in the Celluloid registry.
|
@brandonhilkert great feedback, thank you! I'll let you know when I've made the changes, should be over the next couple days. |
malandrina
force-pushed
the
lw-clear-actor-registry
branch
from
349dc9a to
74a7ce6
Compare
* Currently `SuckerPunch::Railtie` clears all actors from the Celluloid actor registry during initialization. This is problematic because an application using SuckerPunch may have additional Celluloid actors. * In this commit I propose prefixing the names of SuckerPunch actors such that we can ensure the Celluloid registry is cleared only of SuckerPunch actors on application initialization, per @nullstyle's suggestion here: brandonhilkert#99 * Update `SuckerPunch::Queue#name` to add a prefix to queue names * Introduce `SuckerPunch.clear_queues` * Introduce `SuckerPunch::Queue.clear_all` * Update `SuckerPunch::Railtie` to use `SuckerPunch.clear_actors`
malandrina
force-pushed
the
lw-clear-actor-registry
branch
from
74a7ce6 to
7530413
Compare
|
@brandonhilkert this is ready for another look when you have some time. |
|
👍 Thanks again! Nice work! |
brandonhilkert
added a commit
that referenced
this pull request
May 12, 2015
Don't clear non-SuckerPunch actors from registry
|
No problem! Thanks for the opportunity. |
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.
SuckerPunch::Railtieclears all actors from the Celluloidactor registry during initialization. This is problematic because an
application using SuckerPunch may have additional Celluloid actors.
actors such that we can ensure the Celluloid registry is cleared only
of SuckerPunch actors on application initialization, per @nullstyle's
suggestion here: Cannot use celluloid actor registry #99
SuckerPunch::Queue#nameto add a prefix to queue namesSuckerPunch.clear_actorsSuckerPunch::Railtieto useSuckerPunch.clear_actors