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

Implement "isPermaLink" for item:guid #89

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

Conversation

neopostmodern
Copy link

item:guid has to be a URL unless the isPermaLink attribute is set to false.
I've added this functionality targeting RSS 2.0 and updated tests (for which I need to mangle types a bit -- did these tests pass before I checked them out?)
I believe that the behavior for Atom could/should be updated in some way, because it allows non-URL item:id values (Example), but which wouldn't be allowed in RSS 1.0. But this would cause major infrastructure-changes I guess, wheres just adding the isPermaLink attribute should be non-breaking.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 96.316% when pulling 893e463 on neopostmodern:master into 919ad32 on jpmonette:master.

@dainissilamikelis
Copy link

Please add this

@jpmonette jpmonette force-pushed the master branch 2 times, most recently from ebe9963 to fd77835 Compare January 22, 2021 09:47
@senechaux
Copy link

@neopostmodern, I've solved all conflicts in a new PR (#177) to speed up the merge of this feature. I don't want to steal your job or take advantage of it. Please, let me know if you want me to solve conflicts in your master branch so your PR will be ready to merge.

@neopostmodern
Copy link
Author

@senechaux Thanks for your effort. I've just merged the master into my repo again – if that is good to go I'd be happy to use this PR; if it isn't I don't mind using yours instead either.
Most of all we need someone who can merge this – @youssmoh, @jpmonette, who could take care of this?

@senechaux
Copy link

@youssmoh, @jpmonette, is it possible to merge this PR? it's already been approved.

@marco-msg-ferrari
Copy link

Please integrate this 👍

@senechaux
Copy link

@youssmoh, @jpmonette, please, let me know if there is something I can help to merge this PR

@antiantivirus
Copy link

Please integrate this

@mgoldenberg-nv
Copy link

please merge this PR

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.

8 participants