-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add back activesupport #259
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #259 +/- ##
=======================================
Coverage 99.47% 99.47%
=======================================
Files 29 29
Lines 766 767 +1
=======================================
+ Hits 762 763 +1
Misses 4 4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
5263863
to
5994bb4
Compare
We are not ready to remove it. There are instances of `blank?` and `singularize` that are used in the code. Removing `activesupport` still not fails any of the tests, as we have `generator_spec` that uses and installs `activesupport`.
5994bb4
to
5047cfe
Compare
Addresses #260 |
@adomokos also code is using |
@@ -116,7 +116,7 @@ def stop_processing? | |||
end | |||
|
|||
def define_accessor_methods_for_keys(keys) | |||
return if keys.blank? | |||
return if keys.empty? |
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.
@adomokos since we are adding back activesupport
, do we really need to make this change? Instead, we could simply include active_support/core_ext/object/blank
and use the same method without introducing additional changes.
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.
The goal is to yank Active Support dependency, entirely, for pure Ruby projects. Not sure how easily feasible (or how much needed) that is, though.
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.
LGTM
We are not ready to remove it. There are instances of
blank?
andsingularize
that are used in the code.Removing
activesupport
still not fails any of the tests, as we havegenerator_spec
that uses and installsactivesupport
.Fixes #257 and #258.