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

[change] Drop support for depends_on ActiveRecord model #43

Merged
merged 1 commit into from
Feb 17, 2021

Conversation

donaldong
Copy link
Contributor

@donaldong donaldong commented Feb 17, 2021

Summary

This marks depends_on Model, **attrs as a private API. Users can no longer set dependencies on some queries without calling extract_bind_params.

Test Plan

  • ci

@donaldong donaldong changed the base branch from main to base/donaldong/change_drop_suppordbce1db February 17, 2021 05:28
donaldong added a commit that referenced this pull request Feb 17, 2021
Pull Request Branch: donaldong/change_drop_support_for2a4f3a8
Pull Request Link: #43
@donaldong donaldong force-pushed the donaldong/change_drop_support_for2a4f3a8 branch from 50d2aec to f0cfea3 Compare February 17, 2021 05:28
@codecov
Copy link

codecov bot commented Feb 17, 2021

Codecov Report

Merging #43 (8363bba) into main (963d183) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #43      +/-   ##
==========================================
- Coverage   97.10%   97.05%   -0.06%     
==========================================
  Files          32       32              
  Lines        1934     1933       -1     
==========================================
- Hits         1878     1876       -2     
- Misses         56       57       +1     
Impacted Files Coverage Δ
lib/redis_memo/memoizable/dependency.rb 100.00% <100.00%> (ø)
lib/redis_memo/memoize_query/cached_select.rb 94.07% <100.00%> (-0.13%) ⬇️
lib/redis_memo/memoize_query.rb 97.05% <0.00%> (-1.48%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 963d183...cd1f807. Read the comment docs.

@donaldong donaldong changed the title [change] Drop support for depending on ActiveRecord model [change] Drop support for depends_on ActiveRecord model Feb 17, 2021
Comment on lines 35 to 36
memo = model.redis_memo_class_memoizable
nodes[memo.cache_key] = memo
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can move this out of the inner loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that is true!

donaldong added a commit that referenced this pull request Feb 17, 2021
Pull Request Branch: donaldong/change_drop_support_for2a4f3a8
Pull Request Link: #43
@donaldong
Copy link
Contributor Author

move class memo out of the inner loop

donaldong added a commit that referenced this pull request Feb 17, 2021
Pull Request Branch: donaldong/change_drop_support_for2a4f3a8
Pull Request Link: #43
@donaldong donaldong force-pushed the donaldong/change_drop_support_for2a4f3a8 branch from f0cfea3 to 8363bba Compare February 17, 2021 18:50
Pull Request Branch: donaldong/change_drop_support_for2a4f3a8
Pull Request Link: #43
@donaldong donaldong changed the base branch from base/donaldong/change_drop_suppordbce1db to main February 17, 2021 20:59
@donaldong donaldong force-pushed the donaldong/change_drop_support_for2a4f3a8 branch from 8363bba to cd1f807 Compare February 17, 2021 20:59
@donaldong donaldong merged commit e4d5fd0 into main Feb 17, 2021
@donaldong donaldong deleted the donaldong/change_drop_support_for2a4f3a8 branch February 17, 2021 21:00
@katyho katyho mentioned this pull request Feb 22, 2021
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.

None yet

2 participants