Only warn of deprecation if the Rails.env is not test #586
Conversation
|
@maxbeizer IMO these warnings should be shown in tests. They are a primary form of feedback in a lot of folks' development lifecycle. |
|
@joelhawksley yeah that's kind of where I landed on this one. I was looking into whether I could omit then when running the library's tests but ran out of time. That's what I'll be investigating more when I carve out more time to work on this |
maxbeizer
force-pushed
the
maxbeizer/deprecation-warning
branch
2 times, most recently
from
af20c91
to
0ae551c
Compare
The idea here is to wrap the deprecation warnings in a tiny class that uses an env var-- set in test_helper-- to noop VC deprecation warnings in local test mode. This should allow consumers to see deprecation warnings while not showing them to VC contributors.
maxbeizer
force-pushed
the
maxbeizer/deprecation-warning
branch
from
0ae551c
to
cddc367
Compare
| ViewComponent::Deprecation.warn( | ||
| "`preview_path` will be removed in v3.0.0. Use `preview_paths` instead." | ||
| ) |
There was a problem hiding this comment.
👋🏻 what do you think about inlining this check instead of making a new Deprecation class?
| ViewComponent::Deprecation.warn( | |
| "`preview_path` will be removed in v3.0.0. Use `preview_paths` instead." | |
| ) | |
| ActiveSupport::Deprecation.warn( | |
| "`preview_path` will be removed in v3.0.0. Use `preview_paths` instead." | |
| ) unless ENV["VIEW_COMPONENT_ENV"] == "test" |
There was a problem hiding this comment.
I'd say the only argument against it is the following:
We could look at asserting that deprecation warnings invoked. A separate class could-- instead of noop-ing-- retain the warning and allow us to write tests that it would warn outside of local test. So you could write a test that says something like,
# a bit pseudo codey
assert_deprecation_warning "`preview_path`...", preview_path`I certainly get the inclination to avoid the indirection here though. So we could just go back to 927c088 if you prefer.
There was a problem hiding this comment.
The more I think about it, I think ViewComponent::Deprecation is actually a good case for a singleton class too 🤔
There was a problem hiding this comment.
All that said, if you prefer in-lining it then I'm 👍 on that too
| @@ -17,6 +17,7 @@ | |||
|
|
|||
| # Configure Rails Envinronment | |||
| ENV["RAILS_ENV"] = "test" | |||
| ENV["VIEW_COMPONENT_ENV"] = "test" | |||
There was a problem hiding this comment.
As an alternative to setting an environment variable, what are your thoughts on adding a method to ViewComponent::Deprecation like .silence_deprecation_warnings! that we can call early on? That might also give us something to hook into if we wanted to re-enable deprecation warnings for a test later on.
There was a problem hiding this comment.
Interesting idea. I'm still not sure we're going to go down the ViewComponent::Deprecation route but I'll think about it some more 💖
Co-authored-by: Blake Williams <blakewilliams@github.com>
|
I got pulled into other things but I'm hoping to pick this back up soon. Sorry 🙇 |
Summary
related to #585
First pass
I split this one off from the other PR since it may involve more thought/discussion. Is there any reason to show these warnings in test? For development purposes, no. Would library consumers not see this warning with this change? I don't think they will and that may be a bad idea.
Still thinking about it.
Second pass
I think this is a better approach. This uses an env var set up in
test_helperto determine whether we are testing the library or a consumer is running tests against their app. If we are running VC tests, then swallow the warning.In cddc367 decided to introduce a new class to wrap the warning logic. One benefit here is that we could potentially capture those warnings and write tests against the, essentially asserting that various code paths will warn consumers. If you like this approach I can extend it a bit more and fix up the code coverage.
I thought I'd propose it and if you don't like it then we can go back to the simpler 927c088
cc @joelhawksley
Other Information