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

Support all tags #321

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

somini
Copy link

@somini somini commented Sep 14, 2020

Instead of allowing only a subset of tag names, slugify the tag name
so that the URL is always valid.

This is the same thing done on "jekyll-archives":

There is a possibility that the same tag slugifies to the same string, but it's probably fine.

@somini
Copy link
Author

somini commented Nov 1, 2020

This is already done on the latest release.

@somini somini closed this Nov 1, 2020
@somini somini deleted the feature/support-all-tags branch November 1, 2020 20:48
@somini somini restored the feature/support-all-tags branch November 1, 2020 21:00
@somini somini reopened this Nov 1, 2020
@somini
Copy link
Author

somini commented Nov 1, 2020

This is already done on the latest release.

My mistake.

@somini somini force-pushed the feature/support-all-tags branch 2 times, most recently from b9fbcc0 to 4826b55 Compare August 15, 2021 10:59
@somini
Copy link
Author

somini commented Aug 15, 2021

Rebased to the latest master.

@somini
Copy link
Author

somini commented Apr 28, 2022

Rebased to the latest master.

Instead of allowing only a subset of tag names, `slugify` the tag name
so that the URL is always valid.

This is the same thing done on "jekyll-archives":
-
https://github.com/jekyll/jekyll-archives/blob/master/lib/jekyll-archives/archive.rb#L132
@somini
Copy link
Author

somini commented Nov 8, 2022

Rebased to the latest "master.

@gravitystorm
Copy link

I agree that this is a better approach. It's certainly better than silently dropping tags!

I'm not a maintainer here, so I can't recommend what to do next. But my suggestion would be to add some tests (e.g. tags with spaces, tags with underscores, etc), since I wouldn't be comfortable myself merging a PR without them.

Finally, I think we would also need to consider backwards compatibility. At the moment, the feeds have case-sensitive paths, and the tags can contain underscores, and there will be people who have published their feeds like this already, and those feeds will need to keep working.

tag current slugify
foo foo.xml foo.xml
BAR BAR.xml bar.xml
foo_bar foo_bar.xml foo-bar.xml
foo-baz (no output) foo-baz.xml
with space (no output) with-space.xml

I dislike adding configuration options, but maybe it needs a backwards-compatibility flag?

@somini
Copy link
Author

somini commented Jan 23, 2024

I agree, a configuration option for keeping backwards compatibility and some tests would be nice.

I don't think I have much time to implement this, I'm using my branch in production for years now. If you want to take this, be my guest.

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