Skip to content
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

Fix initialize logger #191

Merged
merged 4 commits into from Apr 13, 2016
Merged

Fix initialize logger #191

merged 4 commits into from Apr 13, 2016

Conversation

phstc
Copy link
Collaborator

@phstc phstc commented Apr 12, 2016

Experiment based on #181

end

subject.process(queue, sqs_msg) rescue nil
end

it 're raises the error' do
expect{ subject.process(queue, sqs_msg) }.to raise_error(JSON::ParserError, "757: unexpected token at 'invalid json'")
expect{ subject.process(queue, sqs_msg) }.to raise_error(JSON::ParserError, "784: unexpected token at 'invalid json'")

Choose a reason for hiding this comment

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

Line is too long. [126/120]
Space missing to the left of {.

@phstc phstc changed the title Crowdworks fix initialize logger Fix initialize logger Apr 12, 2016
@phstc phstc force-pushed the crowdworks-fix-initialize-logger branch from 26f8fac to 01f3d6c Compare April 12, 2016 00:57
@@ -15,12 +15,12 @@ def initialize(options)
end

def load
Shoryuken.options.merge!(config_file_options)
Shoryuken.options.merge!(options)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@h3poteto in continuation of #181 (comment). This will logger out in the stdout, as the log wasn't initialized yet. But thinking more on it, I believe it should show an error and exit if the config file supplied is invalid. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it should show an error and exit if the config file supplied is invalid.

I think it is right behavior.
LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks loading the queues from the config when starting with the command-line client.

Basically, the default https://github.com/phstc/shoryuken/blob/master/lib/shoryuken/cli.rb#L115 will cause options to always have a :queues key and clobber whatever is supplied in the config file.

@phstc phstc force-pushed the crowdworks-fix-initialize-logger branch from 01f3d6c to 14ce22b Compare April 12, 2016 01:12
Pablo Cantero added 2 commits April 12, 2016 23:09
…g_file isn't supplied, it's a

continuation of the change in f8f27fc; Replace raise with fail
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.

None yet

4 participants