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

Conversation

pablobm
Copy link
Collaborator

@pablobm pablobm commented Feb 13, 2020

During normal operation, the routes generator can print a few warnings. When there's more than one reason to trigger a warning for a given resource, these are printed out separately, often with other warnings intereleaved.

For example, these are 8 warning about 4 different resources:

WARNING: Unable to generate a dashboard for ActionText::RichText.
         Administrate does not yet support namespaced models.
WARNING: Unable to generate a dashboard for ActionMailbox::InboundEmail.
         Administrate does not yet support namespaced models.
WARNING: Unable to generate a dashboard for ActiveStorage::Blob.
         Administrate does not yet support namespaced models.
WARNING: Unable to generate a dashboard for ActiveStorage::Attachment.
         Administrate does not yet support namespaced models.
WARNING: Unable to generate a dashboard for ActionText::RichText.
         It is not connected to a database table.
         Make sure your database migrations are up to date.
WARNING: Unable to generate a dashboard for ActionMailbox::InboundEmail.
         It is not connected to a database table.
         Make sure your database migrations are up to date.
WARNING: Unable to generate a dashboard for ActiveStorage::Blob.
         It is not connected to a database table.
         Make sure your database migrations are up to date.
WARNING: Unable to generate a dashboard for ActiveStorage::Attachment.
         It is not connected to a database table.
         Make sure your database migrations are up to date.

This is a bit difficult to read and can be confusing. Instead, I propose that we group together the warnings related to each given resource, like so:

WARNING: Unable to generate a dashboard for ActionText::RichText.
       - Administrate does not yet support namespaced models.
       - It is not connected to a database table.
         Make sure your database migrations are up to date.
WARNING: Unable to generate a dashboard for ActionMailbox::InboundEmail.
       - Administrate does not yet support namespaced models.
       - It is not connected to a database table.
         Make sure your database migrations are up to date.
WARNING: Unable to generate a dashboard for ActiveStorage::Blob.
       - Administrate does not yet support namespaced models.
       - It is not connected to a database table.
         Make sure your database migrations are up to date.
WARNING: Unable to generate a dashboard for ActiveStorage::Attachment.
       - Administrate does not yet support namespaced models.
       - It is not connected to a database table.
         Make sure your database migrations are up to date.

This PR implements this behaviour.

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

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.

@nickcharlton nickcharlton merged commit ecae7db into thoughtbot:master Feb 14, 2020
@pablobm pablobm deleted the better-generator-warnings branch June 24, 2021 10:41
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

2 participants