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

Salmon 13 14 acu127014 attributor mvp #2

Merged
merged 15 commits into from
Sep 30, 2013

Conversation

careo
Copy link
Contributor

@careo careo commented Sep 24, 2013

No description provided.

min = options[:min] || 0
max = options[:max] || 1000

rand(max-min) + min
Copy link
Contributor

Choose a reason for hiding this comment

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

This computes the range [min, max) (i.e. max is non-inclusive). Max should not be included?

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 should, although it won't break anything as-is at least.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ya, we should change it to be inclusive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, will wait for this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

acu127014
attribute 'chicken', Chicken
attribute 'duck', Duck
attribute 'turkey', Turkey
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: This is actually broken at the moment because the :email identity does not exist (or actually exists in multiple sub attributes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also didn't really mean to check it in. Whoops.

Copy link
Contributor

Choose a reason for hiding this comment

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

:)

Will wait for this to be updated. The Turducken example seems like it is a valid test case, it is just broken by the :email identity. Maybe replace email with weight.

end


def check_option!(name, definition)
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic code here needs to be documented in the gem README file as well as a method comment above this so users know what they can and cannot do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That, and a whole pile of other documentation, for sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -0,0 +1,96 @@
require 'ostruct'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still required? Don't see any OpenStruct.new usage

magneland pushed a commit that referenced this pull request Sep 30, 2013
…or_mvp

Salmon 13 14 acu127014 attributor mvp
@magneland magneland merged commit 18b20f3 into master Sep 30, 2013
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.

5 participants