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

Adding missing tests for truncate_html_words() #2918

Merged
merged 4 commits into from
Jul 12, 2023

Conversation

galaxy4public
Copy link
Contributor

@galaxy4public galaxy4public commented Sep 8, 2021

Added support for Jinja2 generated end marker when the passed variable
is an instance of Markup. This makes the function compatible with
Jinja2's environments where autoescape is enabled.

Pull Request Checklist

Resolves: #2917

  • 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

Added support for Jinja2 generated end marker when the passed variable
is an instance of Markup.  This makes the function compatible with
Jinja2's environments where autoescape is enabled.
@galaxy4public galaxy4public changed the title Added missing tests for the custom end marker for truncate_html_words() Jinja2's autoescape compatibility for truncate_html_words() Sep 8, 2021
@galaxy4public
Copy link
Contributor Author

I am looking at the failed tests and I am puzzled re: how my change could have affected these three tests. Moreover, when I run tox on a clean clone of the repository (without my changes in), I am also getting these three test failed. At the same time, I see that the latest commit in Jul passed all the tests. Could somebody help me understand what's going on, please?

@justinmayer
Copy link
Member

There are two CI failures. The first one is indeed unrelated to your changes and is instead caused by a newer version of FeedGenerator, for which we have not yet updated our functional test output.

The second one is due to code style conformance checks. Did you follow the development docs and run invoke lint before committing changes? If you make sure you have Pre-commit installed, it should catch those problems even if you forget to run invoke lint.

@justinmayer
Copy link
Member

justinmayer commented Sep 8, 2021

… and the more I look at the diff caused by the latest version of FeedGenerator, the more I wonder whether FeedGenerator 1.9.2 has changed feed output in an undesirable manner. Issue filed: getpelican/feedgenerator#30 🤔

@galaxy4public
Copy link
Contributor Author

Yeah, I also found that FeedGenerator has been updated on Aug 18 and the diff I made shows that the difference is actually the missing <subtitle></subtitle> -- we are passing None as the value, so most likely it was optimised out.

@galaxy4public
Copy link
Contributor Author

@justinmayer , I did fix linting issues (there is a commit just before your comment). I was relying on my Vim being good with flake8 rules, but it was not, so when I've seen the linting errors, I quickly amended the file :).

@galaxy4public
Copy link
Contributor Author

@avaris correctly pointed out that I am tapping into an internal function (truncate_html_words()), which is not exposed to Jinja2 as a filter by default. I agree with that assessment, so I propose to close the corresponding issue (#2917 ). However, I would love to preserve the additional tests introduced in this PR. Will it be OK if I drop the change to truncate_html_words() and the test for it, then rename the PR to be "Adding missing tests for truncate_html_words()" instead? The tests are covering a situation when a custom end_text was provided, e.g. through the SUMMARY_END_SUFFIX parameter in pelicanconf.py.

@justinmayer
Copy link
Member

@galaxy4public: That seems to me a reasonable path forward on this topic. Would you please proceed as you suggested above?

@galaxy4public
Copy link
Contributor Author

@justinmayer , will do that (most likely this coming weekend since I am currently smashed with work :( )

@justinmayer
Copy link
Member

Hi @galaxy4public. Just checking in regarding your pull request. Do you think you could find some time to move it forward so we can get it merged?

@galaxy4public
Copy link
Contributor Author

@justinmayer, sorry, I was overloaded with other work lately. Should be able to work on this by the end of this week.

@galaxy4public galaxy4public changed the title Jinja2's autoescape compatibility for truncate_html_words() Adding missing tests for truncate_html_words() Jul 11, 2023
@galaxy4public
Copy link
Contributor Author

@justinmayer, I've updated the PR and it should be ready to merge.

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 adding these tests, @galaxy4public! 🥇

@justinmayer justinmayer merged commit b8bf595 into getpelican:master Jul 12, 2023
10 checks passed
pauloxnet pushed a commit to pauloxnet/pelican that referenced this pull request Aug 3, 2023
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.

truncate_html_words() breaks with Jinja2's autoescape and the end_text being Markup()
2 participants