Prevent root_dir option from being empty #448
Conversation
When setting root_dir to an empty string (as is done in the default rake task config) this line previously caused root_dir to become an empty array. This in turn caused factories, specs etc not to be annotated. This is just a quick band-aid on a larger problem. We have option parsing spread out over many different places, with slight mismatches like this in the assumptions made.
|
I would probably approach the underlying issue by creating a Then each place that uses these options ( I most likely won't have time to attempt that kind of overhaul, but maybe someone else would like to give it a try? |
lib/tasks/annotate_models.rake
Outdated
| @@ -21,7 +21,7 @@ task annotate_models: :environment do | |||
| options[:show_indexes] = Annotate.true?(ENV['show_indexes']) | |||
| options[:simple_indexes] = Annotate.true?(ENV['simple_indexes']) | |||
| options[:model_dir] = ENV['model_dir'] ? ENV['model_dir'].split(',') : ['app/models'] | |||
| options[:root_dir] = ENV['root_dir'] ? ENV['root_dir'].split(',') : [''] | |||
| options[:root_dir] = ENV['root_dir'] && !ENV['root_dir'].empty? ? ENV['root_dir'].split(',') : [''] | |||
There was a problem hiding this comment.
I wonder if we should fix it where options[:root_dir] being used, make it work whether it's [], [''], or ['', '',...].
There was a problem hiding this comment.
100% yes, we should. :)
I wasn't sure at first of how large the surface of root_dir was, so I wanted to start off with this simple fix. It seems though like this could really easily be taken care of inside AnnotateModels.root_dir.
Would you like me to give that a shot?
These cases are currently handled (inconsistently) in the different option parsing routines.
The different cases are now handled inside AnnotateModels
|
There, I changed the |
I guess, although personally I think I would keep it in the getter. :) Should |
|
As an aside, how do you feel about the idea I mentioned earlier about moving all of this logic into |
I went to do this and then realized why it's problematic: with the getter you'll get I would suggest we keep the current approach. |
* Prevent root_dir option from being empty When setting root_dir to an empty string (as is done in the default rake task config) this line previously caused root_dir to become an empty array. This in turn caused factories, specs etc not to be annotated. This is just a quick band-aid on a larger problem. We have option parsing spread out over many different places, with slight mismatches like this in the assumptions made. * Handle blank values & comma-separated strings in AnnotateModels.root_dir These cases are currently handled (inconsistently) in the different option parsing routines. * Pass root_dir forward unmodified The different cases are now handled inside AnnotateModels
This is a quick fix for #340.
When setting root_dir to an empty string (as is done in the default rake task config) this line previously caused root_dir to become an empty array.
This in turn caused factories, specs etc not to be annotated.
This is just a quick band-aid on a larger problem. We have option parsing spread out over many different places, with slight mismatches like this in the assumptions made.