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

Fix symbolic link handling in pelican-themes -s/-c #3392

Merged
merged 2 commits into from
Sep 28, 2024

Conversation

frederik-elwert
Copy link
Contributor

Pull Request Checklist

  • Ensured tests pass and (if applicable) updated functional test output
  • Conformed to code style guidelines by running appropriate linting tools
  • Added tests for changed code
  • Updated documentation for changed code

Description

Symlinks with relative paths were not handled properly by pelican-themes.

Example:

The pelican theme to be installed is in a sibling directory to the project directory. Running

pelican-themes -s ../my-theme

creates a symlink ../my-theme in pelican’s theme folder. This link is broken, since the relative path in the symlink will now be resolved from pelican’s theme folder.

When running pelican-themes -c from the project folder, it will not detect the broken symlink, since the relative path will actually resolve from the current directory (just not from pelican’s theme folder).

This pull request fixes this by running os.path.realpath on the passed path both when creating symlinks and when checking symlinks. This will make sure that always the absolute paths are linked/checked.

@justinmayer
Copy link
Member

@frederik-elwert: Thank you for the submission!

Any thoughts from @getpelican/reviewers regarding this pull request?

@boxydog
Copy link
Contributor

boxydog commented Sep 21, 2024

Thanks for the contribution.

It would be nice to have a unit test. It might be pretty easy. Whatever broke before.

@frederik-elwert
Copy link
Contributor Author

Thanks for the contribution.

It would be nice to have a unit test. It might be pretty easy. Whatever broke before.

Yes, I agree, it would always be good to have tests. I'm not super experienced with writing tests, and pelican-themes requires file system level interactions with the current Python environment, so I was a bit out of my depth.

@justinmayer justinmayer changed the title Fix symbolic link handling in pelican-themes -s/-c Fix symbolic link handling in pelican-themes -s/-c Sep 28, 2024
Copy link
Member

@justinmayer justinmayer left a comment

Choose a reason for hiding this comment

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

Many thanks for the fix, Frederik. I'm going to merge this now so that we can release a fix for this and thereby eliminate the problem for end users as soon as possible. If you or @boxydog can think of one or more unit tests that might prevent future regressions, submitting them in a follow-up pull request would be most appreciated. Thanks again!

@justinmayer justinmayer merged commit 08283b4 into getpelican:main Sep 28, 2024
15 of 16 checks passed
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.

4 participants