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

Instantiate middleware #118

Merged
merged 1 commit into from
Jul 20, 2020
Merged

Conversation

ChrisBr
Copy link
Collaborator

@ChrisBr ChrisBr commented May 13, 2020

So far we only created one instance of the middleware and then operated on this instance. This made the code quite ugly because we always needed to pass around the method parameters from method to method. This PR creates now a new instance for each request.

The main part what I changed is basically from

class Subscriber
  def call(name, start, finish, payload)
  end
end

to

class Subscriber
  def self.call(name, start, finish, payload)
    new(name, start, finish, payload)
  end
end

which resulted that we basically dropped all method parameters from all functions. In my opinion this makes the code a lot nicer to work with because we can easily extract helper methods and memoize values. This refactoring is a preparation for #117 which made it quite hard to squeeze more code without the ability to have helper methods.

Downside of this approach is of course that we create a few more instances per request but I think this is worth it.

@ChrisBr ChrisBr requested a review from hennevogel May 13, 2020 22:48
@ChrisBr ChrisBr force-pushed the cb/move-config branch 2 times, most recently from 02f4d4d to e84d5be Compare May 13, 2020 22:54
}.each do |hook_name, subscriber_class|
subscribe_to(hook_name, subscriber_class)
}.each do |hook_name, subscriber|
subscribe_to(hook_name, subscriber)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the subscribe_to method brings any clarity anymore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah good point 👍 Lets get rid of it!

@hennevogel hennevogel merged commit 35b4f2b into influxdata:master Jul 20, 2020
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.

2 participants