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

Fixes reference for schema_format to AR::Base from AS::Base #19176

Merged
merged 1 commit into from
Mar 3, 2015

Conversation

imajes
Copy link
Contributor

@imajes imajes commented Mar 2, 2015

Hey-

this is a simple fix, but i think it should also get backported to the relevant stable branches in the 4.x series. I don't think it is relevant for 3.x.

@sgrif
Copy link
Contributor

sgrif commented Mar 2, 2015

This is missing a test case.

@imajes
Copy link
Contributor Author

imajes commented Mar 2, 2015

Sure, what do you want to test for? it's essentially a typo fix that obviously didn't have a test to begin with

@sgrif
Copy link
Contributor

sgrif commented Mar 2, 2015

Calling the method with no arguments so the default is invoked.

@imajes imajes force-pushed the master branch 2 times, most recently from d9227d8 to 110dc67 Compare March 2, 2015 21:15
@imajes
Copy link
Contributor Author

imajes commented Mar 2, 2015

Fair 'nuff. This should be good to go now then.


File.expects(:join).with('/tmp', 'schema.rb')
ActiveRecord::Tasks::DatabaseTasks.schema_file
end

Choose a reason for hiding this comment

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

Isn't better to just assert the result of schema_file? Wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

👍 on checking the returned path.

@imajes
Copy link
Contributor Author

imajes commented Mar 3, 2015

Yeah that's reasonable to check for return. All the other tests were checking expectations so I kinda just jumped on that bandwagon. This should now be all good, plus i threw in a couple tests to exercise both format variations, just for good measure.

senny added a commit that referenced this pull request Mar 3, 2015
Fixes reference for schema_format to AR::Base from AS::Base
@senny senny merged commit b0edabf into rails:master Mar 3, 2015
@senny
Copy link
Member

senny commented Mar 3, 2015

thank you 💛

@imajes
Copy link
Contributor Author

imajes commented Mar 3, 2015

@senny, can we apply this to Rails 4 branches too? I actually noticed it because I was bitten by it this week switching between :schema and :structure. Seems like it's somewhat a regressive change between the 3.x and 4.x branches.

@senny
Copy link
Member

senny commented Mar 4, 2015

@imajes sure, I merged from my phone 😁

senny added a commit that referenced this pull request Mar 4, 2015
senny added a commit that referenced this pull request Mar 4, 2015
Fixes reference for schema_format to AR::Base from AS::Base
Conflicts:
	activerecord/lib/active_record/tasks/database_tasks.rb
@senny
Copy link
Member

senny commented Mar 4, 2015

@imajes backported to 4-2-stable. 4-1-stable did not have this issue.

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

4 participants