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

Continuation of #273 #276

Merged
merged 12 commits into from
Dec 3, 2016
Merged

Continuation of #273 #276

merged 12 commits into from
Dec 3, 2016

Conversation

phstc
Copy link
Collaborator

@phstc phstc commented Dec 3, 2016

Fixes #272

return unless fifo?

options[:message_group_id] ||= MESSAGE_GROUP_ID
options[:message_deduplication_id] ||= Digest::SHA256.digest(options[:message_body])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@richseviora so in here I think we can set message_group_id and message_deduplication_id in case they are not supplied. I want to avoid checking has_content_deduplication?, otherwise we would need do a request to check if it's set or not per message, like N+1. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point and agreed :)


if options[:delay_seconds]
fail ArgumentError, 'FIFO queues do not accept DelaySeconds arguments.'
end
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@richseviora I removed other validation, because this code will prevent that to be nil anyways. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. The code was something I put in place really for dev purposes, it's definitely redundant now.

@phstc phstc mentioned this pull request Dec 3, 2016
This usage of the Code Climate Test Reporter is now deprecated. Since
version
      1.0, we now require you to run `SimpleCov` in your test/spec
helper, and then
      run the provided `codeclimate-test-reporter` binary separately to
report your
      results to Code Climate.
      More information here:
https://github.com/codeclimate/ruby-test-reporter/blob/master/README.md
https://travis-ci.org/phstc/shoryuken/jobs/180890771
@phstc
Copy link
Collaborator Author

phstc commented Dec 3, 2016

Pablo Cantero added 3 commits December 2, 2016 22:59
Those were previously added because the AWS SDK error wasn't very descriptive.

Current errors are very descriptive:

- ArgumentError: expected params[:message_body] to be a String, got value
10 (class: Fixnum) instead.
- ArgumentError: FIFO queues do not accept DelaySeconds arguments.
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