Skip to content

Conversation

AlfonsoUceda
Copy link
Contributor

reference #72

Be careful before merging because the lotus-router is pointing to my fork and an especific branch related with this hanami/hanami-router#36 (later this will be restored to lotus/router github)

Thanks you @jodosha for the work

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling d0c4fb0 on AlfonsoUceda:action_middleware into 0893ab5 on lotus:master.

@runlevel5
Copy link

Looks good 👍

@AlfonsoUceda
Copy link
Contributor Author

I need to create want test for rack builder in a couple of hours. Yesterday I forgot it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this correct?

UseAction::Index.rack_builder == UseAction::Show.rack_builder # => true
UseAction::Index.rack_builder.must_equal UseAction::Show.rack_builder
# => Fail
# No visible difference in the Rack::Builder#inspect output.
# You should look at the implementation of #== on Rack::Builder or its members.
UseAction::Index.rack_builder.object_id.must_equal UseAction::Show.rack_builder.object_id # => false

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this would add a value. We're testing a detail, instead of a behavior. Also, if Rack Team customizes Rack::Builder#== this test will fail in the future.

The behavior that we want to describe is: given two actions with separated middleware stacks, those shall not to interfers each other.

What if we introduce YMiddleware and let UseAction::Show to use it? Then we could introspect the response and headers returned from /show.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, then I'll rewrite the test ;)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 99.09% when pulling 45888ca on AlfonsoUceda:action_middleware into 0893ab5 on lotus:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 99.09% when pulling 99d7317 on AlfonsoUceda:action_middleware into 0893ab5 on lotus:master.

Copy link
Member

Choose a reason for hiding this comment

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

This is a low level detail that we can safely remove from here. The test already covers it. 🐱

@jodosha jodosha self-assigned this Jan 23, 2015
@jodosha jodosha added this to the v0.3.2 milestone Jan 23, 2015
@jodosha
Copy link
Member

jodosha commented Jan 23, 2015

@AlfonsoUceda This looks good! ✨ 👍 for me.

@AlfonsoUceda
Copy link
Contributor Author

@jodosha thanks for your help ;)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 99.09% when pulling bcbff7a on AlfonsoUceda:action_middleware into 0893ab5 on lotus:master.

@runlevel5
Copy link

@AlfonsoUceda ❇️ 👍 great work

jodosha added a commit that referenced this pull request Jan 24, 2015
Refactorized action rack implementation
@jodosha jodosha merged commit ae0da31 into hanami:master Jan 24, 2015
@AlfonsoUceda AlfonsoUceda deleted the action_middleware branch May 15, 2015 09:21
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.

4 participants