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

Move from proxies to well-defined adapters #441

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Move from proxies to well-defined adapters #441

wants to merge 1 commit into from

Conversation

fatkodima
Copy link
Contributor

This is a work in progress (failing tests, missing new tests and documentation), but basically is ready for review to verify basic ideas.

To add new adapters, all is needed is inheriting from base class and implementing required methods:

module Rack
  class Attack
    class MyStoreAdapter < StoreAdapter
      ...

Closes #380

grzuy
grzuy reviewed Oct 23, 2019
View reviewed changes
Copy link
Collaborator

@grzuy grzuy left a comment

Choose a reason for hiding this comment

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

Thank you for working on this one!

lib/rack/attack/store_adapters/store_adapter.rb Outdated
class << self
def inherited(klass)
@adapters << klass
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

Will just this inherited piece suffice to add support to "add your own cache backend store", by making proxies lookup dynamic instead of hard-coded?

If so, I'll prefer to have that as a small first step, and then re-consider the other changes.

Copy link
Contributor Author

@fatkodima fatkodima Oct 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes.

If so, I'll prefer to have that as a small first step, and then re-consider the other changes.

You mean extracting as a separate PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes.

Awesome.

If so, I'll prefer to have that as a small first step, and then re-consider the other changes.

You mean extracting as a separate PR?

Yes :-)

@fatkodima fatkodima mentioned this pull request Oct 24, 2019
Merged
@fatkodima fatkodima changed the title Move from proxies to well-defined adapters [skip ci] Move from proxies to well-defined adapters Oct 25, 2019
@fatkodima
Copy link
Contributor Author

Updated this accordingly to master. Added tests. This pr became a little bold.
@grzuy Please, review.

@grzuy
Copy link
Collaborator

grzuy commented Jan 14, 2020

This can be considered backwards incompatible change, so it should be considered for v7.x.

@fatkodima
Copy link
Contributor Author

Hey, @grzuy
Wdyt on moving this (and my other PRs) forward? Is it time for v7.x?

@ioquatix ioquatix deleted the branch rack:main February 1, 2022 20:05
@ioquatix ioquatix closed this Feb 1, 2022
@ioquatix ioquatix reopened this Feb 1, 2022
@ioquatix ioquatix changed the base branch from master to main February 1, 2022 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grzuy grzuy grzuy left review comments

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.

Move from proxies to well-defined adapters
3 participants
@fatkodima @grzuy @ioquatix