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

Provide option to have an uncompressed sitemap index. #124

Merged
merged 5 commits into from
Feb 5, 2014

Conversation

mitchless
Copy link

For our sitemap we wanted to have the first sitemap.xml uncompressed. To this end, I have added the option "gzip_initial" to SitemapGenerator::Sitemap. By default, this is "true." If set to false, the first sitemap file will be left uncompressed.

@kjvarga
Copy link
Owner

kjvarga commented Sep 6, 2013

What's your reasoning for only uncompressing the first file? I've been interested in allowing the compression to be an option but in my experience everyone has different needs so I've left it off. But I like how you've modified the FileAdapter and Namer. Those changes make it easy to add an option for this.

I was thinking to have the option :compress and support values true, false, :both (writes out both). Any idea for a value that would make sense for your case? But first I want to hear of your rationale...:all_but_first could work, but I imagine it would be easy to forget it

@mitchless
Copy link
Author

We had a terrible time getting Bing to recognize our sitemap until we presented an uncompressed version. Everything else could be compressed, but unless that first file was uncompressed we had no success with Bing.

It also allows us to read the index more easily, which we have surprisingly done from time to time.

As for a :compress value, :all_but_first works for me.

@PikachuEXE
Copy link

I need Bing support too
And I don't want to hack the gem to do so :P

@ACPK
Copy link

ACPK commented Dec 11, 2013

So Bing doesn't allow compressed sitemaps?

@kjvarga
Copy link
Owner

kjvarga commented Dec 11, 2013

This feature is top of my list. And is long overdue. I need to put some time aside to address it. Unfortunately I'm rather busy these days. Perhaps over the Christmas break.

@mjc
Copy link

mjc commented Dec 31, 2013

Perhaps it would be good to generate both? this would also allow nginx's gzip_static to work - it looks for the uncompressed version and sends the pre-compressed one when Accept-Encoding: gzip is sent by the client.

@PikachuEXE
Copy link

I suggest just generate both and have an option later (if adding an options takes more time)

@kjvarga
Copy link
Owner

kjvarga commented Jan 6, 2014

Hi guys, great news! I have implemented this feature! I've released gem v5.0.0.beta. It is based off of this branch: https://github.com/kjvarga/sitemap_generator/tree/uncompressed-sitemaps

Can you try it out and make sure everything works as expected. I'm pretty confident all is okay.

I was able to implement these option values (see the README in that branch for more info):

  • true
  • false
  • :all_but_first

Generating both was actually more difficult because of how things are set up. It is possible, but will have to come at a later date. Besides, now that you can generate uncompressed sitemaps it's pretty trivial to write a couple lines at the end of your config to go through and write out compressed versions of the files.

@kjvarga
Copy link
Owner

kjvarga commented Jan 7, 2014

The problem with generating both is which file(s) should the links in the sitemap index(es) point to? The compressed or uncompressed versions? Should the uncompressed index point to compressed sitemaps? Then there would be no reference to the uncompressed sitemaps, making them somewhat useless? Different people would expect different things. So until we can clarify this issue I'll leave it off.

@kjvarga
Copy link
Owner

kjvarga commented Jan 7, 2014

I just released 5.0.0.beta1 which fixes problems with the summary output. I've confirmed things are all good on my end now.

@kjvarga
Copy link
Owner

kjvarga commented Jan 22, 2014

Anyone have any update on testing out the beta?

@PikachuEXE
Copy link

Not tested yet, Will try to test on Friday

@PikachuEXE
Copy link

Tested locally with all 3 values. Seems working fine!
Test later on production

@kjvarga kjvarga merged commit 60a6fae into kjvarga:master Feb 5, 2014
@kjvarga
Copy link
Owner

kjvarga commented Feb 5, 2014

Released in v5.0

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.

None yet

5 participants