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.

Sign up for GitHub

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

Jump to bottom

Fix compatibility with AS Concern #26

Merged
merged 4 commits into from
Mar 26, 2020
Merged

Fix compatibility with AS Concern #26

merged 4 commits into from
Mar 26, 2020

Conversation

tycooon
Copy link
Owner

@tycooon tycooon commented Mar 24, 2020

Fixes the following exception when using Memery in a ActiveSupport::Concern module:

ArgumentError:
  wrong number of arguments (given 0, expected 1)
# ./lib/memery.rb:15:in `included'

@tycooon tycooon force-pushed the fix-included-method-overriding branch from 956ee26 to 354041e Compare March 24, 2020 15:49
@tycooon
Copy link
Owner Author

tycooon commented Mar 24, 2020

@AlexWayfer what do you think? It's pretty common to use both AS::Concern and Memery in the same module, so I think that we should better drop that auto-include feature.

AlexWayfer
AlexWayfer suggested changes Mar 25, 2020
View reviewed changes
Copy link
Contributor

@AlexWayfer AlexWayfer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done changes to this branch with saving compatibility of previous abilities (without reverting). Wait for a PR, please.

lib/memery.rb
@@ -32,16 +27,14 @@ def memoized?(method_name)
return false unless defined?(@_memery_module)

@_memery_module.method_defined?(method_name) ||
@_memery_module.private_method_defined?(method_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like extra (non-related) change.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, but I'm usually OK with some minimal cosmetic changes :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but let's add the RuboCop to the CI of this repository and check if it's OK.

Can we switch from Travis CI? I have no permissions to do this, and I suggest Cirrus CI or Circle CI (a config of the first one will be shorten).

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There actually is rubocop in the CI. It gets called in the bundle exec rake command.

And btw what's the problem with Travis?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There actually is rubocop in the CI. It gets called in the bundle exec rake command.

Oh, OK, then it's strange that both approaches (with indentation and without) are acceptable.

And btw what's the problem with Travis?

I had many issues with environment, but if it satisfies you — never mind. Also, it uses the deprecated method of integration with GitHub, as I know.

Also: …

image

@AlexWayfer
Copy link
Contributor

Done. You can merge #27 into this branch and then merge this PR if everything is OK.

@tycooon tycooon merged commit d956418 into master Mar 26, 2020
@tycooon tycooon deleted the fix-included-method-overriding branch March 26, 2020 10:06
@tycooon tycooon mentioned this pull request Mar 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexWayfer AlexWayfer AlexWayfer requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tycooon @AlexWayfer