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

Ignore non active record subclasses #50

Merged

Conversation

markburns
Copy link
Contributor

Fixes #44, #47, #48

private

def safe_write
File.open(mock_file_path, 'w') do |file_out|
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@markburns
Copy link
Contributor Author

My first encounter with the hound bot. It seems quite useful, but I wonder if you've thought of using rubocop with --autocorrect?

A lot of these issues were already present, but I'm happy to fix them.
Shall I add some rubocop config so you can tweak to your preferred style?

else
MockCreator.enabled_partials_default
end
FileUtils.mkdir_p(config.mock_dir) unless Dir.exists?(config.mock_dir)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dir.exists? is deprecated in favor of Dir.exist?.

@zeisler
Copy link
Owner

zeisler commented Apr 28, 2016

I've deleted all the style violations about line length.

@zeisler
Copy link
Owner

zeisler commented Apr 29, 2016

Thank you for your pull request. I will have time to review it this weekend.

@markburns
Copy link
Contributor Author

BTW, the main changes were done in 137dae1

It is probably worth reviewing that as the intent of the PR. The other stuff is refactoring work mainly to make method simpler/classes shorter etc triggered by hound/rubocop.

I can re-do a PR with just those commits if you like

constant = model_name.constantize
return unless constant.ancestors.include?(ActiveRecord::Base)
constant
rescue StandardError, LoadError => e
Copy link
Owner

Choose a reason for hiding this comment

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

String#constantize can cause a NameError, what's the case for LoadError. I know StandardError will be a catch all, but I like the thought of calling out specific know failure scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I don't want to be a smart-ass, but you'd have to answer that yourself :)

rescue StandardError, LoadError => e

Some of the renaming I've done here was just for clarifying, and obviously makes the PR a little harder to parse, so apologies about that.

For reference: http://blog.nicksieger.com/articles/2006/09/06/rubys-exception-hierarchy/

So having NameError, LoadError instead makes sense. The previous code was just not as precise as it could have been.

I'll add that change in here, as it absolutely makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The above issue has been addressed, but is not showing up in the Github UI as a comment on an outdated diff.

@markburns markburns force-pushed the ignore_non_active_record_subclasses branch from 86c85d0 to a316b74 Compare May 1, 2016 06:53
@@ -126,6 +148,7 @@

before do
ActiveMocker::Config.progress_bar = false
ActiveMocker::Config.model_dir = File.join(File.expand_path("../../", __FILE__), "models")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line is too long. [106/80]

describe "ActiveMocker::Config.disable_modules_and_constants" do

before do
ActiveMocker::Config.disable_modules_and_constants = set_to
ActiveMocker::Config.progress_bar = false
ActiveMocker::Config.model_dir = File.join(File.expand_path("../../", __FILE__), "models")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line is too long. [118/80]

@markburns
Copy link
Contributor Author

OK the issues have been addressed in 524fa38

@markburns markburns force-pushed the ignore_non_active_record_subclasses branch from 524fa38 to 6f83d71 Compare May 1, 2016 23:50
@@ -122,7 +144,8 @@
describe "ActiveMocker::Config.mock_append_name" do
before do
ActiveMocker::Config.progress_bar = false
ActiveMocker::Config.single_model_path = File.join(File.expand_path("../../models", __FILE__), "model.rb")
ActiveMocker::Config.model_dir = File.expand_path("../../models", __FILE__)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line is too long. [91/80]

@markburns
Copy link
Contributor Author

This is rebased against latest master, but the history is getting messy. If you've reviewed it and are interested in merging it in I can squash if you like?

@zeisler zeisler merged commit 214dca2 into zeisler:master May 2, 2016
@markburns markburns deleted the ignore_non_active_record_subclasses branch May 2, 2016 05:09
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