-
Notifications
You must be signed in to change notification settings - Fork 67
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
Relax activesupport dependency #236
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
require 'active_support/deprecation' | ||
require 'structured_warnings' | ||
|
||
module LightService | ||
module Action | ||
|
@@ -9,7 +9,7 @@ def self.extended(base_class) | |
def self.included(base_class) | ||
msg = "including LightService::Action is deprecated. " \ | ||
"Please use `extend LightService::Action` instead" | ||
ActiveSupport::Deprecation.warn(msg) | ||
warn(StructuredWarnings::DeprecatedMethodWarning, msg) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here and wherever we thrown a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm completely for that course of action. |
||
base_class.extend Macros | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,9 @@ Gem::Specification.new do |gem| | |
gem.required_ruby_version = '>= 2.6.0' | ||
|
||
gem.add_runtime_dependency("activesupport", ">= 4.0.0") | ||
gem.add_runtime_dependency("i18n") | ||
gem.add_runtime_dependency("dry-inflector") | ||
gem.add_runtime_dependency("structured_warnings") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I missed these additions. So instead of having just AS, we are adding 3 more dependencies. I wonder what ppl think about this change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We’re adding only 2 more dependencies, since i18n is already in through AS at the moment. I explicitly required it as a dep in order to have a functioning gemspec even considering AS removal and without leaving remainders for future work. Speaking about the balance between AS and the two new gems: my sentiment is that with the two gems we bring less code than with AS alone. We should analyze it more scientifically tho. For sure we have the achievement of not having a big rails ecosystem’s gem, leaving LS more desirable as cross-framework and franework-less solution. Speaking of deprecation warnings: I previously tried to implement them using Gem::Deprecate, but it resulted to me as an inadequate tool out of its absolute standard concept. Hard to adapt. I think it would be feasible to write a dedicated Logger and implement some logic inside it in order to achieve a simple custom deprecation warner. This would be intended as a pragmatic and simpler alternative to structured_warning gem. About the inflector I’m really opinionated: I would not even try to reinvent the wheel on such a matter. Moreover I sincerely invite you to read dry-inflector’s source code: it’s astonishing small, tidy and simple; and it’s a completely standalone gem. Obviously we could copy that few methods we need, but pro and cons considered I think it’s a solid choice to bring it in. Thanks for the feedback 😊 |
||
|
||
gem.add_development_dependency("generator_spec", "~> 0.9.4") | ||
gem.add_development_dependency("test-unit", "~> 3.0") # Needed for generator specs. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,29 +1,30 @@ | ||
require 'spec_helper' | ||
require 'structured_warnings_helper' | ||
|
||
describe "Including is discouraged" do | ||
context "when including LightService::Organizer" do | ||
it "gives warning" do | ||
expected_msg = "including LightService::Organizer is deprecated. " \ | ||
"Please use `extend LightService::Organizer` instead" | ||
expect(ActiveSupport::Deprecation).to receive(:warn) | ||
.with(expected_msg) | ||
|
||
class OrganizerIncludingLS | ||
include LightService::Organizer | ||
end | ||
expect do | ||
class OrganizerIncludingLS | ||
include LightService::Organizer | ||
end | ||
end.to warn_with(StructuredWarnings::DeprecatedMethodWarning, expected_msg) | ||
Comment on lines
+10
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests are great! We should remember to remove them when the actual removal takes place. 👍🏻 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given this discussion #236 (comment) it's still to take a decision about the use of that library. That said and considering that whatever implementation will be it will have a test: remembering to remove deprecation from code will bring a red CI. That should be a good post-it to remove the test too ;) |
||
end | ||
end | ||
|
||
context "when including LightService::Action" do | ||
it "gives warning" do | ||
expected_msg = "including LightService::Action is deprecated. " \ | ||
"Please use `extend LightService::Action` instead" | ||
expect(ActiveSupport::Deprecation).to receive(:warn) | ||
.with(expected_msg) | ||
|
||
class ActionIncludingLS | ||
include LightService::Action | ||
end | ||
expect do | ||
class ActionIncludingLS | ||
include LightService::Action | ||
end | ||
end.to warn_with(StructuredWarnings::DeprecatedMethodWarning, expected_msg) | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
require 'structured_warnings/test' | ||
|
||
module StructuredWarningHelper | ||
module_function | ||
|
||
def parse_arguments(args) # rubocop:disable Metrics/MethodLength, Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity | ||
args = args.clone | ||
first = args.shift | ||
|
||
if first.is_a?(Class) && first <= StructuredWarnings::Base | ||
warning = first | ||
message = args.shift | ||
|
||
elsif first.is_a?(Class) && !(first <= StructuredWarnings::Base) | ||
raise ArgumentError, 'Warning issued with a class not inheriting from StructuredWarnings::Base' | ||
|
||
elsif first.is_a? StructuredWarnings::Base | ||
warning = first.class | ||
message = first.message | ||
|
||
elsif first.is_a? String | ||
warning = StructuredWarnings::StandardWarning | ||
message = first | ||
|
||
else | ||
warning = StructuredWarnings::Base | ||
message = nil | ||
end | ||
|
||
unless args.empty? | ||
raise ArgumentError, | ||
"wrong number of arguments (#{args.size + 2} for 2)" | ||
end | ||
|
||
return warning, message | ||
end | ||
|
||
def args_inspect(args) | ||
args.map(&:inspect).join(', ') | ||
end | ||
end | ||
|
||
RSpec::Matchers.define :warn_with do |*args| | ||
supports_block_expectations | ||
warning, message = StructuredWarningHelper.parse_arguments(args) | ||
|
||
match do |block| | ||
w = StructuredWarnings::Test::Warner.new | ||
StructuredWarnings.with_warner(w, &block) | ||
expect(w.warned?(warning, message)).to be true | ||
end | ||
|
||
failure_message do |_block| | ||
"<#{StructuredWarningHelper.args_inspect(args)}> has not been emitted." | ||
end | ||
|
||
failure_message_when_negated do |_block| | ||
"<#{StructuredWarningHelper.args_inspect(args)}> has been emitted but it was not expected." | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,5 @@ | ||
RSpec.shared_context 'expect orchestrator warning' do | ||
before do | ||
expect(ActiveSupport::Deprecation) | ||
.to receive(:warn) | ||
.with(/^`Orchestrator#/) | ||
.at_least(:once) | ||
around(:example) do |example| | ||
expect { example.run }.to warn_with StructuredWarnings::DeprecatedMethodWarning | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're requiring
structured_warnings
inlight-service.rb
we probably don't need to re-require them again in the rest of the files, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totaly agree: OR in the manifest OR in single files. In both places is just confusing and conceptually wrong.