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

add gzip compressor #303

Merged
merged 1 commit into from
Dec 11, 2012
Merged

add gzip compressor #303

merged 1 commit into from
Dec 11, 2012

Conversation

tkellen
Copy link
Contributor

@tkellen tkellen commented Dec 10, 2012

added for ease-of-use with nginx serving gzipped data directly from memcached

@mperham
Copy link
Collaborator

mperham commented Dec 10, 2012

Tests?

@tkellen
Copy link
Contributor Author

tkellen commented Dec 10, 2012

Added. I believe the test is set up correctly, can you confirm?

describe 'GzipCompressor' do

should 'compress and uncompress data using Zlib::GzipWriter/Reader' do
memcached(19127,nil,{:compress=>true,:compressor=>Dalli::GzipCompressor}) do |dc|
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change :compressor to :compessor and watch the spec still pass. You need to set some expectations to verify your methods are being called.

@tkellen
Copy link
Contributor Author

tkellen commented Dec 11, 2012

Roger that, please see the test now.

@mperham
Copy link
Collaborator

mperham commented Dec 11, 2012

Did you forget to push? There's no new commit.

@tkellen
Copy link
Contributor Author

tkellen commented Dec 11, 2012

@tkellen
Copy link
Contributor Author

tkellen commented Dec 11, 2012

To be more precise, I force pushed over my existing branch to update the PR.

mperham added a commit that referenced this pull request Dec 11, 2012
@mperham mperham merged commit 4252f98 into petergoldstein:master Dec 11, 2012
@mperham
Copy link
Collaborator

mperham commented Dec 11, 2012

Got it, thank you!

@mperham
Copy link
Collaborator

mperham commented Dec 11, 2012

Oh, if you want to update the changelog with a note about nginx / gzip compatibility, I bet some people would find that useful.

@tkellen
Copy link
Contributor Author

tkellen commented Dec 11, 2012

Roger that, coming up.

@tkellen
Copy link
Contributor Author

tkellen commented Dec 11, 2012

Do you want it listed in History.md under 2.6.0?

@mperham
Copy link
Collaborator

mperham commented Dec 11, 2012

That would be fine.

@tkellen
Copy link
Contributor Author

tkellen commented Dec 11, 2012

Not sure why, but the PR button isn't showing up for this, feel free to grab it manually!
https://github.com/tkellen/dalli/tree/docs

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