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

Abstract model classes should be skipped without warning. #582

Closed
wants to merge 2 commits into from

Conversation

lukeredpath
Copy link

@lukeredpath lukeredpath commented May 16, 2016

I’ve also clarified the existing specs by including an assertion against the presence of a warning, so the name of the spec matches what is actually being tested.

I'm not 100% happy with checking against an exact match on the warning text but it seemed important to check for the presence (or absence) of a warning.

This is related to #581 but I'm not sure its the only issue I'm having with this generator on Rails 5.

I’ve also clarified the existing specs by including an assertion against
the presence of a warning, so the name of the spec matches what is 
actually being tested.
it 'skips abstract models without a warning' do
stub_generator_dependencies
routes = file("config/routes.rb")

Choose a reason for hiding this comment

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

Trailing whitespace detected.

@BenMorganIO BenMorganIO self-assigned this May 25, 2016
@BenMorganIO
Copy link
Collaborator

@lukeredpath totally want to get this merged. Can you rebase and squash 8e945d6 and 4551dc6 into a single commit? You could also use heredocs to help shorten the strings; or multilines with \. :)

@carlosramireziii
Copy link
Collaborator

@lukeredpath Just wanted to check in on this one.. Would you be able to take care of the feedback from above? We'd love to get this one merged. Thanks!

@nickcharlton
Copy link
Member

@lukeredpath Could I get you to fix up the Hound warnings on this and rebase? Or alternatively, enable contributor pushes?

@nickcharlton
Copy link
Member

I'm going to close this as I've rebased myself and opened #857.

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