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

Add language attribute to KalibroConfiguration #110

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

danielkza
Copy link
Contributor

@danielkza danielkza commented May 30, 2016

Fixes #109.

@rafamanzo
Copy link
Member

Nice work. Just missed the CHANGELOG :)

@@ -17,6 +17,14 @@
require 'spec_helper'

describe KalibroClient::Entities::Configurations::KalibroConfiguration do
describe 'accessors' do
it 'should have accessors for the needed attributes' do
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't we just testing the way ruby is supposed to work here?

If this is for documentation purposes (which I think is good), maybe we should consider printing more specific messages.

What do you think of:

[:name, :description, :language].each do |attr|
  it "should have accessors for the #{attr} attribute" do
    expect(subject).to have_attr_accessor(attr)
  end
end

Caveat: I'm not entirely sure that code snippet works 🍷

Choose a reason for hiding this comment

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

It does :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test is just to ensure the methods are defined. It doesn't check where
they actually work. I thought it was interesting, since it is the defining
part of the change, and defines the API.

The have_attr_accessor matcher unfortunately isn't part of rspec, but I
imported it from a StackOverflow answer.

Em sáb, 4 de jun de 2016 21:57, Eduardo Silva [email protected]
escreveu:

In spec/entities/configurations/kalibro_configuration_spec.rb
#110 (comment):

@@ -17,6 +17,14 @@
require 'spec_helper'

describe KalibroClient::Entities::Configurations::KalibroConfiguration do

  • describe 'accessors' do
  • it 'should have accessors for the needed attributes' do

It does :)


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/mezuro/kalibro_client/pull/110/files/70427ae757120cfd73f7e51077190a42b8010cfe#r65809835,
or mute the thread
https://github.com/notifications/unsubscribe/AAtnTnZwp_Rica8ZCWBvV5DK7fOtqWP1ks5qIh7igaJpZM4IqDsX
.

Copy link
Contributor

Choose a reason for hiding this comment

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

So that reinforces my opinion that we should use the code snippet I wrote.

Thanks for checking that it works, @dread-uo!

danielkza and others added 4 commits June 10, 2016 15:27
Since we use tests as documentation, it is nice to have more informative
messages.
Signed-off-by: Daniel Miranda <[email protected]>
@do-you-dare do-you-dare force-pushed the kalibro_configuration_language branch from e590f6d to bd78b44 Compare June 10, 2016 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants