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.

Sign up for GitHub

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

Jump to bottom

Set correct configuration method name and add tests #19

Merged
merged 5 commits into from
May 10, 2016
Merged

Set correct configuration method name and add tests #19

merged 5 commits into from
May 10, 2016

Conversation

gauravtiwari
Copy link
Contributor

No description provided.

@gauravtiwari gauravtiwari changed the title Update configuration block and support up to rack 2.0 Update configuration block and support rack 2.0 May 6, 2016
@gauravtiwari gauravtiwari force-pushed the master branch 7 times, most recently from a42dac1 to 07eeae1 Compare May 7, 2016 05:18
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 99.171% when pulling eedd417 on gauravtiwari:master into e26eccc on hyperoslo:master.

@jgorset
Copy link
Owner

jgorset commented May 10, 2016 •
edited
Loading

Nice, @gauravtiwari! Two things, though:

1: This pull request has two different things in it, so it should be two pull requests; one for Rack 2.0, and one for the configuration block.

2: I like that you can configure without a block now! I don't think it makes as much sense to read configuration values with Facebook::Messenger.configure.access_token, though. It sort of works for writing configuration values because it's a verb. If you renamed configure to configuration, I guess it would make sense for both reading and writing. What do you think?

Also, thanks for adding tests!

@jgorset
Copy link
Owner

jgorset commented May 10, 2016 •
edited
Loading

Actually, no, configuration doesn't read as well with the block format. Consider the following:

# Conventional, reads well
Facebook::Messenger.configure do |config|
  config.access_token = '...'
end

# Unconventional, doesn't read well because it's not a verb and you're changing something
Facebook::Messenger.configuration do |config|
  config.access_token = '...'
end

# Conventional, reads well
Facebook::Messenger.configuration.access_token = '...'

# Unconventional, but still reads pretty well because it's a verb and you're changing something
Facebook::Messenger.configure.access_token = '...'

# Unconventional, doesn't read well because it's a verb and you're not changing anything
Facebook::Messenger.configure.access_token # => '...'

@gauravtiwari
Copy link
Contributor Author

gauravtiwari commented May 10, 2016 •
edited
Loading

@jgorset Yup, I thought so when making a PR, but may be forgot along the way. I will revert the rack change and make two pull requests. Yes makes sense. Should I add another method for module self.configuration for reading? Earlier you had self.config I guess that would have worked too :)

@jgorset
Copy link
Owner

jgorset commented May 10, 2016 •
edited
Loading

I guess config works because it's short for both. I think I'd prefer that over having two methods configure and configuration because it's the convention that people have come to expect and because it avoids having two methods that do the same thing.

@gauravtiwari
Copy link
Contributor Author

@jgorset Sure thing, self.config makes sense. Guess, I messed it 😛

@jgorset
Copy link
Owner

jgorset commented May 10, 2016

Leave the tests, though! I messed that. 😁

@gauravtiwari
Copy link
Contributor Author

Will do 👍

@gauravtiwari
Copy link
Contributor Author

gauravtiwari commented May 10, 2016 •
edited
Loading

@jgorset You tried this: https://github.com/hyperoslo/facebook-messenger-demo, basically here you have to message: https://www.facebook.com/chatbotmessenger/ (added you as a tester)

@gauravtiwari
Support configuration block
a66fab5 
Fix wordings

Fix quotes

Test yield configuration

Fix syntax

Fix rubocop offenses

Fix tests

Refactor tests

Fix rubocop offense

Use configure
@gauravtiwari
Use configure to set configuration
6a59fdb
@gauravtiwari
Update readme
7eb6365
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 99.169% when pulling 9a37975 on gauravtiwari:master into e26eccc on hyperoslo:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 99.169% when pulling e63884e on gauravtiwari:master into e26eccc on hyperoslo:master.

@gauravtiwari gauravtiwari changed the title Update configuration block and support rack 2.0 Update configuration method name and add tests May 10, 2016
@gauravtiwari
Copy link
Contributor Author

@jgorset Guess this is it for this PR?

@gauravtiwari gauravtiwari changed the title Update configuration method name and add tests Set correct configuration method name and add tests May 10, 2016
@frodsan
Copy link
Contributor

frodsan commented May 10, 2016 •
edited
Loading

Why not leave #configure? It's very clear IMO and it doesn't break compatibility. Rails does the same: https://github.com/rails/rails/blob/04ad814bb992e854927c1bec3422df882f017ab6/activesupport/lib/active_support/configurable.rb#L36

PD: I will only leave #configure in the README, and leave #config for advanced users.

@jgorset
Copy link
Owner

jgorset commented May 10, 2016 •
edited
Loading

@frodsan is right, of course – configure and config is the convention. We should probably keep it for that reason alone, not to mention that we'll also break compatibility if we change it. Sorry for all the back and forth, @gauravtiwari; too much on my plate today, I think. 😉

jgorset
jgorset reviewed May 10, 2016
View reviewed changes
lib/facebook/messenger/server.rb
when @request.get? then verify
when @request.post? then receive
if @request.get? then verify
elsif @request.post? then receive
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer case to elsif in general, but especially if there's a chance there might be more conditions down the road. I couldn't find anything on this in bbatsov/ruby-style-guide, though – what do you think, @hyperoslo/web?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jgorset Code wise, they both look the same except the extra case statement, same for readability. Rubocop now, doesn't like case.

Copy link
Owner

@jgorset jgorset May 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed it doesn't! It doesn't like empty case statements at all. I yield, then!

@jgorset
Copy link
Owner

jgorset commented May 10, 2016 •
edited
Loading

If you revert to configure and config I'll totally merge this already, @gauravtiwari. Sorry. 😝

@gauravtiwari
Copy link
Contributor Author

@jgorset All reverted already 😄

@gauravtiwari
Copy link
Contributor Author

Ohh wait! config

@gauravtiwari
Copy link
Contributor Author

@jgorset Just take a look once if all looks good 😃

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 99.174% when pulling 34f887c on gauravtiwari:master into e26eccc on hyperoslo:master.

jgorset
jgorset reviewed May 10, 2016
View reviewed changes
README.md
Facebook::Messenger.config.verify_token
# => my_voice_is_my_password_verify_me
```

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure that we need this since the configuration is really only used internally, but we can remove it later.

@jgorset
Copy link
Owner

jgorset commented May 10, 2016

Looks good! Nice job!

@jgorset jgorset merged commit 8b62993 into jgorset:master May 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gauravtiwari @coveralls @jgorset @frodsan