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 failing test for double exposures #56 #124

Closed
wants to merge 2 commits into from

Conversation

Morred
Copy link

@Morred Morred commented Apr 29, 2015

#56

@marshall-lee
Copy link
Member

Hello, @Morred!

I closed & reopened your PR just to initiate a re-run of Travis CI checks. Build still fails but this time it's just due to RuboCop offenses. RSpec tests pass fine as you can see here: https://travis-ci.org/intridea/grape-entity/jobs/73909288 so issue #56 can be considered as solved.

I will not merge this PR because new specs from #151 for this issue are sufficient. If you don't think so and want this to be merged please somehow fix RuboCop offenses.

spec/grape_entity/entity_spec.rb:338:52: C: Use the new lambda literal syntax ->(params) {...}.

            expose :media, using: SoundEntity, if: lambda { |object, _| object.sound? }

                                                   ^^^^^^

spec/grape_entity/entity_spec.rb:339:52: C: Use the new lambda literal syntax ->(params) {...}.

            expose :media, using: PhotoEntity, if: lambda { |object, _| object.photo? }

                                                   ^^^^^^

I personally don't like this type of RuboCop offense about lambda syntax and if you think the same feel free to open a separate PR with patch for .rubocop.yml.

I also don't like defining constants like PhotoEntity in specs because these things are global and survive among rspec examples. I know there are some specs in entity_spec.rb that already define constants but it's something that I want to get rid of. I already had issues with constants while working on this project. @dblock, what do you think? Also I need to say that this problem is gracefully solved in https://github.com/Casecommons/with_model gem but it's only for ActiveRecord models. I also seen a good workaround for this in Her gem, please look at https://github.com/remiprev/her/blob/49cd15ffbad2aec104810753a90d9a7b6977fd31/spec/support/macros/model_macros.rb#L6 and https://github.com/remiprev/her/blob/c39fbb6d211109da5f2ff8c35fff4b5a85d8d5c9/spec/spec_helper.rb#L21

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