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

safely wrap title: contents using json dumps #23

Closed
wants to merge 3 commits into from

Conversation

maphew
Copy link
Contributor

@maphew maphew commented Feb 4, 2023

This should handle any arbitrary punctuation marks which may happen to be in the Title - ",',\,*,...etc.

I don't undersand why this works, since we're not doing anything with json. I just know that everything I've thrown at it comes out safely.

I left the change from #15 intact, though json.dumps makes it unnecessary. Using them both at the same time doesn't seem to change results. I've tested and verified that " don't become double escaped

Pull Request Checklist

Resolves: #20

  • Conformed to code style guidelines by running appropriate linting tools

Note: as mentioned in closed PR #22 I can't run the linting during commit since my system hates it. For this PR I edited and tested and linted search.py on my local machine and then manually copied the changes via Github website editor. However I can't guarantee some mistake did not happen in the middle.

  • Updated documentation for changed code

Doc updates believed unnecessary.

maphew and others added 2 commits February 4, 2023 14:01
This should handle any arbitrary punctuation marks which may happen to
be in the Title - `",',\,*,...etc`.

I don't undersand why this works, since we're not doing anything with 
json. I just know that everything I've thrown at it comes out safely.

I left the change from pelican-plugins#15 intact, though json.dumps makes it 
unnecessary.
Copy link
Contributor Author

@maphew maphew left a comment

Choose a reason for hiding this comment

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

I don't understand why the pre-commit black has a problem. I ran black locally first and it reported 3 files, unchanged. I guess I have to dig and learn why the pre-commit hook hates my system so I can run that locally.

...by removing manual title cleaning of nested quotes.
Linting test is successful: commit was blocked until the now unused
`title` variable was removed.

My earlier statement was incorrect: processing `title` with the manual
quote handling AND dumps *does* result in double escaping of quotes.
@maphew
Copy link
Contributor Author

maphew commented Feb 9, 2023

Status: contrary to opening comment #15 was removed in the end since the changes did conflict. IMO this PR is better than 15 in that it handles many special characters in title and not just ("), but whatever you decide is fine. (No more changes from me planned.)

@justinmayer
Copy link
Contributor

I manually merged this change and released it in version 1.0.2. Many thanks for the improvement, Matt. 🏅

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.

Missing index.st (search plugin breaks parsing titles with " or \)
2 participants