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

Warn when precompiling assets in development #102

Merged
merged 1 commit into from
Aug 28, 2022

Conversation

abevoelker
Copy link
Contributor

Per #97 this adds a warning when doing rails assets:precompile in development

I don't know what the preferred wording is, and also the default logging in a fresh Rails 7 app seems to be writing this to log/development.log but not emitting it to the console's STDERR - not sure if that is desirable?

@abevoelker abevoelker marked this pull request as ready for review August 3, 2022 01:39
@brenogazzola
Copy link
Collaborator

Thanks @abevoelker.

Since the message explicitly uses the word "precompiling" and "serving assets" I'm not sure placing it in the compiler is the best choice. If we have something other the assets:precompile using that method later, the message would not make much sense.

Let's follow the example of the reveal feature and move the message to the task itself and use a standard p. This would ensure it's printed to the terminal and always visible to the developer, even if he chose to have his logs somewhere else.

@abevoelker
Copy link
Contributor Author

@brenogazzola Thanks, that makes sense. Let me know if the force-pushed change is more like what you were thinking of. Should it be a standard puts like the reveal feature which writes to stdout, or perhaps warn or $stderr.puts that go to stderr since it's a warning? Or perhaps it doesn't matter. At a glance I see a mix in the main Rails code base.

@brenogazzola brenogazzola merged commit 7df6247 into rails:main Aug 28, 2022
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.

2 participants