Refactor loads and authorize resource #525
Conversation
| through_as_symbols.map { |tas| "@#{tas}" }.join(" || ") | ||
| eval "@#{options[:polymorphic]} ||= #{possible_sources_of_parent}" | ||
| unless instance_variable_defined?("@#{options[:polymorphic]}") | ||
| parent = through_as_symbols.lazy.filter_map { instance_variable_get "@#{_1}" }.first |
There was a problem hiding this comment.
Just to demonstrate how lazy.filter_map {}.first works. It's so we only loop through just the amount of calls we need to find and return a match:
irb(main):004> [1, 2, 3, 4].filter_map { p _1; _1 if _1.even? }.first
1
2
3
4
=> 2
irb(main):005> [1, 2, 3, 4].lazy.filter_map { p _1; _1 if _1.even? }.first
1
2
=> 2
|
@jagthedrummer I've tried cut down on some the incidental complexity in |
|
@kaspth I don't know a whole lot about this class so it may take me a little time to give this a thorough review. |
|
@kaspth, I'm having a hard time making sense of this PR. Partly because I don't know much about this class, but also partly because it does several refactorings all at once and it's not clear to me which changes are related to which other changes. Could we break it down into a few smaller ones? Like could we do one PR for replacing |
|
@jagthedrummer hey yeah, good point. I'll try to get back to this, this week. |
|
@kaspth, just wanted to check in on your thoughts on this one. I've been reading the current version of that method, and I think I have at least a little more context as to what all is going on this PR, and I don't see anything that's obviously concerning. But I do still think it would be nice to split each logical change into smaller PRs. I can take a stab at that if you're not able to get to it. |
|
@jagthedrummer oh, I forgot about this! I'll take a look at splitting this up tomorrow. |
``` account_load_and_authorize_resource :project, :team # New account_load_and_authorize_resource :project, through: :team # Old ``` The deprecate notice TODO was added over 2 years ago, and Bullet Train is still using the old form. Don't think its happening. c5ba33b
In the previous commit I removed `.constantize` from `begin`, but `.klass` above is what loads the model and is why we can call `name` on it. So I don't think this exception is ever hit.
I'm grouping it with the exception so it's easier to see why `through_as_symbols.first` works.
`eval` just evaluates in the same scope, so if we can assign instance variables in there, we can both get and assign them without eval.
kaspth
force-pushed
the
refactor-loads-and-authorize-resource
branch
from
bc66500 to
fcd755a
Compare
|
@jagthedrummer ok, I've split all of these out into 5 different PRs. I've stacked them so part-2 merges into part-1 etc. So if you merge them in order, GitHub should point the PR next-in-line branch back to This should let you sequence and ship these in several different releases if you want to slow the rollout down a bit. |
I'm trying to see if there's some fairly simple and safe refactoring we could do here. Just because this method is 140 lines long.