Skip to content

Take article_url into account for articles, translated articles, and pages#41

Closed
gregsadetsky wants to merge 2 commits into
pelican-plugins:mainfrom
gregsadetsky:main
Closed

Take article_url into account for articles, translated articles, and pages#41
gregsadetsky wants to merge 2 commits into
pelican-plugins:mainfrom
gregsadetsky:main

Conversation

@gregsadetsky

@gregsadetsky gregsadetsky commented Oct 10, 2025

Copy link
Copy Markdown
Contributor

Fixes #32.

the plugin was using the filesystem path to generate urls instead of respecting the ARTICLE_URL and PAGE_URL settings. so if you had ARTICLE_URL set to "blog/{slug}/" but ARTICLE_SAVE_AS as "{slug}/index.html", the sitemap would show example.com/test-post/ when it should show example.com/blog/test-post/. same issue for pages with custom PAGE_URL settings.

instead of deriving the url from the filesystem path using to_url(path), we just use obj.url directly for articles and pages since Pelican already computed the correct url respecting the defined settings. for index pages like archives and tags we still use to_url(path) since the related objects don't have a url property.

also fixed the same issue in translation alternate links. those were using trans.save_as when they should use trans.url for the same reason.

added tests to cover the custom ARTICLE_URL scenario, custom PAGE_URL scenario, and translation links with custom urls. these two latter code paths did not have test coverage before.

all existing tests still pass and linting is clean.

cheers!

@gregsadetsky gregsadetsky changed the title take article_url into account for articles and translated articles take article_url into account for articles, translated articles and pages Oct 10, 2025
@justinmayer
justinmayer requested a review from kernc October 11, 2025 06:15
@justinmayer justinmayer changed the title take article_url into account for articles, translated articles and pages Take article_url into account for articles, translated articles, and pages Oct 12, 2025

@justinmayer justinmayer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey Greg! Many thanks for your contribution, which seems well-considered and thorough — and even replete with test coverage. Bravo! Much appreciated. 🌟

As I mentioned in the first segment of a recent episode of the Abstractions podcast, I prefer to reserve project-level .gitignore changes for project-specific entries, with more general entries (such as __pycache__/) going into the global ~/.gitignore file on developers’ individual workstations. Also, even if these kinds of changes were welcomed, they would be best submitted in the context of a separate commit and pull request. So I have removed that change and have merged your other changes manually.

Finally, in the future I suggest creating a new feature branch instead of making your changes in your fork’s main branch. This will make things a bit easier for maintainers if and when they need to make changes directly in the context of your pull request.

Thanks for listening to these very minor suggestions for the future. Once again, thank you very much for your work on this!

@justinmayer justinmayer added the manual merge Pull request was merged manually label Oct 12, 2025
@justinmayer

Copy link
Copy Markdown
Contributor

@kernc: Many thanks for reviewing. Very helpful. Much obliged!

@justinmayer

Copy link
Copy Markdown
Contributor

These changes were released in Sitemap plugin version 1.2.2

@gregsadetsky

gregsadetsky commented Oct 12, 2025

Copy link
Copy Markdown
Contributor Author

amazing, thanks a lot for the review, the merge and the new version release!

thanks for the note re: feature branch (my bad) and the .gitignore (which makes total sense - my spidey sense lit up for a second when I saw that the file contained almost none of the usual python gitignore entries - I should have listened to it ha)

happy to also confirm that my blog (which uses pelican & this plugin) is now back to not having 404s in its sitemap :-) which was the goal all along :-)

cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

manual merge Pull request was merged manually

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Invalid URLs generated

3 participants