-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove ARGV
use during runtime
#65
Conversation
@@ -35,11 +35,11 @@ def self.parse(args, existing_options = {}) | |||
}.freeze | |||
|
|||
def initialize(args, existing_options) | |||
@args = args | |||
@args = args.clone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this change, @args
was a reference to ARGV
. This resulted in parse_command
and parser.parse!
to modify ARGV
.
|
||
options = config_file_options.merge(parsed_options) | ||
|
||
@options = Options.from(options, {}) | ||
@options = Options.from(options, {working_args: remaining_args}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could probably come up with a better name
|
||
# Note: This is currently broken as we don't set `is_rake` anywhere. | ||
# It's an artifact from the old Annotate gem and how it did control flow. | ||
model_files = list_model_files_from_argument(options) if !options[:is_rake] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:is_rake
is a legacy option from the old Annotate gem, it's not used in this gem.
This PR removes
ARGV
from being used implicitly during runtime. Before this change, ARGV would be used and modified inParser#parse
which made it hard to reason about howModelFilesGetter#list_model_files_from_argument
works.ARGV
is still used at the time of execution but gets cloned inParser#new
.