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

Fresh pass at displaying more detail during linting #707

Merged
merged 1 commit into from Oct 17, 2014

Conversation

joshuaclayton
Copy link
Contributor

Closes #692, #664, #696, #691

factories_to_lint.inject({}) do |result, factory|
begin
FactoryGirl.create(factory.name)
rescue => e
Copy link
Member

Choose a reason for hiding this comment

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

e => error ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 896ae8b

@jferris
Copy link
Member

jferris commented Oct 17, 2014

Worth including the exception class name?

Looks good, generally. Ready to merge at your discretion.

@joshuaclayton
Copy link
Contributor Author

@jferris the exception's class name could be valuable - especially if the message itself is ambiguous.

I've changed the output to:

* user
  ActiveRecord::RecordInvalid: Validation failed: Name can't be blank
* admin_user
  ActiveRecord::RecordInvalid: Validation failed: Name can't be blank

Thoughts?

@joshuaclayton
Copy link
Contributor Author

maybe a parenthetical after the factory with the exception class name, with the message underneath?

* user (ActiveRecord::RecordInvalid)
  Validation failed: Name can't be blank
* admin_user (ActiveRecord::RecordInvalid)
  Validation failed: Name can't be blank

@jferris
Copy link
Member

jferris commented Oct 17, 2014

I'm guessing all on one line is generally okay. Would make it more convenient for things like grep anyway.

@joshuaclayton joshuaclayton force-pushed the jc-linting-error-messages-round-2 branch from ca451af to f451e4a Compare October 17, 2014 20:40
@joshuaclayton joshuaclayton merged commit f451e4a into master Oct 17, 2014
@joshuaclayton joshuaclayton deleted the jc-linting-error-messages-round-2 branch October 17, 2014 20:42
@silatham99
Copy link

This new behavior is inconsistent with the .lint documentation. Calling .create on any factory with dependent records is going to violate foreign key constraints.

@joshuaclayton
Copy link
Contributor Author

@silatham99 I've updated docs to better reflect how linting operates.

The intention of Factory Girl is to be able to create data, so defined factories should in fact be able to be created.

alext added a commit to alphagov/content-register that referenced this pull request Oct 31, 2014
Since thoughtbot/factory_bot#707 the linter now
calls create on each factory, and this was polluting the database.
Wrapping this in a transaction that's rolled back prevents this.
alext added a commit to alphagov/content-register that referenced this pull request Nov 3, 2014
Since thoughtbot/factory_bot#707 the linter now
calls create on each factory, and this was polluting the database.
Wrapping this in a transaction that's rolled back prevents this.
alext added a commit to alphagov/content-register that referenced this pull request Nov 3, 2014
Since thoughtbot/factory_bot#707 the linter now
calls create on each factory, and this was polluting the database.
Wrapping this in a transaction that's rolled back prevents this.
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.

None yet

3 participants