Skip to content

Accept wrapper default params #283

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

Merged
merged 2 commits into from
Oct 30, 2015

Conversation

kamilbielawski
Copy link
Contributor

This PR allows setting default wrapper_start and wrapper_end options for rake task.
I didn't include wrapper option here. While it's useful as a shorthand when setting options directly from the command line, I think it might be unnecessarily confusing here. If you don't agree, I can include it in the rake task as well.

PR fixes issue #266

@kamilbielawski kamilbielawski force-pushed the accept-wrapper-default-params branch from e3c1501 to 2c01787 Compare October 26, 2015 22:04
@kamilbielawski
Copy link
Contributor Author

Hmm, it looks like Semaphore isn't configured properly, doesn't it?

@@ -78,6 +78,12 @@ def self.setup_options(options = {})
return options
end

def self.reset_options
[POSITION_OPTIONS, FLAG_OPTIONS, PATH_OPTIONS, OTHER_OPTIONS].flatten.each do |key|
ENV[key.to_s] = nil
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason to set these to nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's needed for the tests - ENV wasn't cleared between tests, so after wrapper attributes were set in one of the tests, the others also had it set, which resulted in incorrect annotations being generated.

@ctran
Copy link
Owner

ctran commented Oct 29, 2015

Please ignore the failure from semaphoreci, I didn't have the correct configuration.

ctran added a commit that referenced this pull request Oct 30, 2015
@ctran ctran merged commit 9cf6e0d into ctran:develop Oct 30, 2015
@ctran
Copy link
Owner

ctran commented Oct 30, 2015

Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants