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

More readable warnings #1551

Merged
merged 1 commit into from Feb 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 12 additions & 12 deletions lib/generators/administrate/routes/routes_generator.rb
Expand Up @@ -20,15 +20,15 @@ def insert_dashboard_routes
end

def warn_about_invalid_models
namespaced_models.each do |invalid_model|
puts "WARNING: Unable to generate a dashboard for #{invalid_model}."
puts " Administrate does not yet support namespaced models."
end

models_without_tables.each do |invalid_model|
puts "WARNING: Unable to generate a dashboard for #{invalid_model}."
puts " It is not connected to a database table."
puts " Make sure your database migrations are up to date."
invalid_dashboard_models.each do |model|
puts "WARNING: Unable to generate a dashboard for #{model}."
if namespaced_models.include?(model)
puts " - Administrate does not yet support namespaced models."
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a different question to ask is: Do we still need this warning? In general, we do support namespaced models. What's missing from the current support which means we should still have this warning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly. I got here because I was looking into that. The error messages were confusing, so I thought of fixing that first, then looking into generating those namespaces. My thinking is that getting this to work properly may (perhaps) take long, so meanwhile let's at least make this more bearable, which is low-hanging fruit.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, let's do it!

end
if models_without_tables.include?(model)
puts " - It is not connected to a database table."
puts " Make sure your database migrations are up to date."
end
end

unnamed_constants.each do |invalid_model|
Expand All @@ -49,15 +49,15 @@ def dashboard_resources
end

def valid_dashboard_models
database_models - invalid_database_models
database_models - invalid_dashboard_models
end

def database_models
ActiveRecord::Base.descendants.reject(&:abstract_class?)
end

def invalid_database_models
models_without_tables + namespaced_models + unnamed_constants
def invalid_dashboard_models
Copy link
Member

Choose a reason for hiding this comment

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

This is a nice change!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, the inconsistent naming was bothering me too.

(models_without_tables + namespaced_models + unnamed_constants).uniq
end

def models_without_tables
Expand Down
18 changes: 17 additions & 1 deletion spec/generators/routes_generator_spec.rb
Expand Up @@ -46,7 +46,7 @@

expect(output).to include <<-MSG.strip_heredoc
WARNING: Unable to generate a dashboard for Blog::Post.
Administrate does not yet support namespaced models.
- Administrate does not yet support namespaced models.
MSG
end

Expand Down Expand Up @@ -97,6 +97,22 @@ class AbstractModel < ApplicationRecord
"dashboard for AbstractModel")
end
end

it "groups together warnings related to the same model" do
class TestModelNamespace
class ModelWithoutDBTable < ApplicationRecord; end
end

model_name = TestModelNamespace::ModelWithoutDBTable.to_s
output = run_generator

warning = "WARNING: Unable to generate a dashboard " \
"for #{model_name}."
occurrences = output.scan(warning).count
expect(occurrences).to eq(1)
ensure
remove_constants :TestModelNamespace
end
end

it "creates a root route for the admin namespace" do
Expand Down