Skip to content

Conversation

@backus
Copy link
Member

@backus backus commented Aug 23, 2015

Failing test for issue mentioned in #22. Good to go @DelmerGA

@backus backus force-pushed the fix/person-question_sets-array branch from 13263f7 to 8df17e4 Compare August 23, 2015 02:36

Choose a reason for hiding this comment

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

Style/CollectionMethods: Prefer map over collect.

@backus backus force-pushed the fix/person-question_sets-array branch from daf4e25 to 53ee411 Compare August 26, 2015 18:42

Choose a reason for hiding this comment

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

Style/HashSyntax: Use the new Ruby 1.9 hash syntax.

@backus backus force-pushed the fix/person-question_sets-array branch from 06d9c39 to 723e402 Compare August 31, 2015 20:01

Choose a reason for hiding this comment

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

Style/SpaceInsideBlockBraces: Space missing inside }.

@OfTheDelmer OfTheDelmer force-pushed the fix/person-question_sets-array branch from 7bddfa5 to 3f3e142 Compare September 8, 2015 17:31
John Backus added 4 commits September 8, 2015 11:50
Currently devtools is adding the rspec-its dependency, but only for rubies
>= 2.1, so we add our own explicit dependency in our gemspec for earlier rubies
Lets you specify how many question set ids should come with the
person resource you are creating
@backus backus force-pushed the fix/person-question_sets-array branch from b60fb2e to 9911cf2 Compare September 8, 2015 18:50

Choose a reason for hiding this comment

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

Style/IfUnlessModifier: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.

Choose a reason for hiding this comment

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

Style/SpaceInsideBlockBraces: Space missing inside {.

Refactors specs related to collection from question sets to separate
spec and moves some member related specs out of collection specs. Still
requires more specs on Member to raise mutant coverage. It also renames
the Base#saved? to Base#persisted?.
@OfTheDelmer OfTheDelmer force-pushed the fix/person-question_sets-array branch from b46e959 to a4dd8e1 Compare September 10, 2015 17:58
@OfTheDelmer OfTheDelmer force-pushed the fix/person-question_sets-array branch 2 times, most recently from 36a6c5e to 28d55e5 Compare September 10, 2015 23:53
@OfTheDelmer OfTheDelmer force-pushed the fix/person-question_sets-array branch from 28d55e5 to 999a725 Compare September 11, 2015 00:00
Copy link
Member Author

Choose a reason for hiding this comment

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

pry

@backus
Copy link
Member Author

backus commented Sep 11, 2015

In general we try to follow the best practices for rspec. Mainly I'd recommend checking out https://github.com/howaboutwe/rspec-style-guide http://betterspecs.org/. A few specific points I would point out:

  1. Prefer short describes, short its, and long contexts. Usually describe and it will just be a constant name or a method description like '.foo' or '#bar'. Same for it minus the constant. This isn't a strict rule since sometimes it is hard to avoid, but in general this is preferable and it also factors into how mutant works with your specs

  2. Similar to the note above, feel free to omit it descriptions when possible. For example:

    describe '#all' do
      subject { collection.all  }
    
      it 'should return self' do
        is_expected.to be(collection)
      end
    end

    vs.

    describe '#all' do
      subject { collection.all }
      it { should be(collection) }
    end
  3. You can name your subjects like so:

    subject(:foo) { something_else.get_foo }

    This way a spec like the following

    subject { collection.create }
    
    it 'should retrieve item from registered' do
      found_qs = collection.retrieve(subject.id)
      expect(subject).to eq(found_qs)
    end

    could be

    subject(:member) { collection.create }
    
    it 'should retrieve item from registered' do
      found_qs = collection.retrieve(member.id)
      expect(subject).to eq(found_qs)
    end

    which I think is much more readable

Choose a reason for hiding this comment

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

Style/Documentation: Missing top-level module documentation comment.

@OfTheDelmer OfTheDelmer force-pushed the fix/person-question_sets-array branch from bcdb257 to 59b6a29 Compare September 14, 2015 21:59
OfTheDelmer added a commit that referenced this pull request Sep 14, 2015
@OfTheDelmer OfTheDelmer merged commit 243bdc7 into master Sep 14, 2015
@OfTheDelmer
Copy link
Contributor

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants