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] Match all occurrences of {static} / {attach} #3420

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mart-e
Copy link
Contributor

@mart-e mart-e commented Nov 3, 2024

If a markup element contains more than one {static} or {attach} value, only the last one was replaced.

Simplify the regex to match every occurrence.
This regex no longer checks for < at the start which may imply false positive (to test if acceptable or not).

Detail of the issue: #3419

@pauloxnet
Copy link
Member

Can you add a test?

@mart-e mart-e marked this pull request as ready for review November 3, 2024 16:56
@justinmayer
Copy link
Member

justinmayer commented Nov 3, 2024

@mart-e: There is a test failure, which needs to be addressed. If that test failure is due to the updated regex now actually correctly replacing the second {static} reference (and please make sure that is actually the case — I have not looked at length), then you can update the functional test output as described in the docs.

@mart-e
Copy link
Contributor Author

mart-e commented Nov 3, 2024

@justinmayer no that test failure is the test I am adding, looks like my patch is actually not working/not enough. I have some troubles to invoke the test suit on my machine (probably some setup issues).
Is there a way to use a debugger like pudb?

@justinmayer
Copy link
Member

Let's start with getting the tests running on your local workstation… What have you done so far, and what seems to be the problem with running the tests?

If a markup element contains more than one {static} or {attach} value,
only the last one was replaced.

Simplify the regex to match every occurrence

Fixes getpelican#3419
@justinmayer
Copy link
Member

Looks like your test now passes. Bravo, Martin! Well done 👏

@justinmayer justinmayer changed the title [FIX] match all occurrences of static/attach [FIX] Match all occurrences of {static} / {attach} Nov 3, 2024
@mart-e
Copy link
Contributor Author

mart-e commented Nov 3, 2024

Ok, I have figured out what was the issue, missing file declaration in my new test.

What have you done so far, and what seems to be the problem with running the tests?

I used invoke setup to be sure my env is up to date and then invoke test on the main branch. The test_pelican.py is failing. The output is very long (output), my terminal doesn’t have enough scroll back to reach the beginning but the interesting part seems to be that for every file, I get

E           diff --git a/home/mat/other/pelican/pelican/tests/output/basic/feeds/all.atom.xml.orig b/home/mat/other/pelican/pelican/tests/output/basic/feeds/all.atom.xml.orig
E           new file mode 100644
E           index 00000000..5552b0ea
E           --- /dev/null
E           +++ b/home/mat/other/pelican/pelican/tests/output/basic/feeds/all.atom.xml.orig

@justinmayer
Copy link
Member

justinmayer commented Nov 3, 2024

I do not believe there should be any […].orig files in the Pelican repository, so those are likely leftover detritus of some Git operation. I suggest using something like fd, particularly with its command execution option, to remove those files. Perhaps something like:

cd ~/projects/pelican
fd --hidden --glob '*.orig' --exec trash

(Above requires you have some kind of trash executable such as https://github.com/andreafrancia/trash-cli or https://github.com/sindresorhus/macos-trash that can safely remove files without potentially-risky permanent deletion.)

@mart-e
Copy link
Contributor Author

mart-e commented Nov 3, 2024

Oh you are right, much better when purging those files, I can run the tests now. Thanks a lot for your help 👍

As a sidenote, I am no longer the initial example with two attributes containing {static} for lazyloading images on my blog (as it required javascript to work).
I still think this change is valid but in case you have a doubts, feel free to reject this PR as the usecase is probably rare.

Or maybe somebody better than me in regex can find a way to achieve the same while still checking the starting <?

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.

3 participants