Skip to content
This repository has been archived by the owner on May 2, 2020. It is now read-only.

Allow user to specify headers #3

Merged
merged 1 commit into from
Feb 26, 2015
Merged

Allow user to specify headers #3

merged 1 commit into from
Feb 26, 2015

Conversation

michaldarda
Copy link
Contributor

Fixes #1

Hello @emq ;)

Hope you find it useful

Just one thing, I don't know it is just me or it is simply broken

  1) My Middleware logging without storage raises an error
     Failure/Error: expect { get '/' }.to raise_error(Rack::RequestPolice::NoStorageFound)
       expected Rack::RequestPolice::NoStorageFound but nothing was raised
     # ./spec/middleware_spec.rb:222:in `block (3 levels) in <top (required)

I also had to modify configuration in other specs, because class variables were cached between specs and test result were undetermistic.

Fixes #1

Add feature to allow users specify what headers they want to save in
storage. In addition it is possible to set custom name under which
header will be kept in storage and apply any transformations on it.
@rwojsznis
Copy link
Owner

Hi Michał, long time no code :trollface:

Thanks for the PR, looks neat 👍 it seems like this spec you mention passes on travis, oh boy 😁 will try to take a look before releasing new version on rubygems.


header_value = env[header_name]
# apply transformations to header_value, fail silently on any exception
header_value = transformation.call(header_value) rescue header_value
Copy link
Owner

Choose a reason for hiding this comment

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

Not a big fan of inline rescue - what's the reason behind it?

Can you give me real-world usage example please? I'm guessing there is more to 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.

Well, me neither

But..

If user specifies wrong transformation, for example

{ |h| h.some_not_existing_method }

Seems reasonable to me, to rescue to original header_value if that transformation fails instead of returning 500 ;)

Copy link
Owner

Choose a reason for hiding this comment

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

We're transforming a string there, so personally I would rather get 500 instantly than to swallow exception silently (bad experience with too much rescue :trollface: ) - but within current implementation this is indeed not possible - transformation is always called, even if header is missing (you transform nil, rescue it if something bad happens (that returns nil again) and then fallback to fallback value (I hope I didn't mess up anything, but it seems about right). It might be a personal taste, but I will allow myself to modify this a little 😉

Except that - thanks again for PR 👍 I hope I will manage to release new version this week, this is probably a good time to add a changelog too ;-).

Cheers ❤️

rwojsznis added a commit that referenced this pull request Feb 26, 2015
@rwojsznis rwojsznis merged commit c083a0c into rwojsznis:master Feb 26, 2015
@rwojsznis
Copy link
Owner

note: was chilling in Slovakia, will try to release this upcoming week, sorry for the delay ;*

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to log chosen headers
2 participants