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.

Sign up for GitHub

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

Jump to bottom

Implement --invert to remove tags matching selector. #81

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Implement --invert to remove tags matching selector. #81

wants to merge 3 commits into from

Conversation

wolfgang42
Copy link

Fixes #74. I've never written any Go before, so I'd appreciate a code review. Marked WIP because of that and also because I want to add tests but haven't looked at the test harness hard enough to figure out how yet.

@wolfgang42
Copy link
Author

The code I wrote has some sort of bug. Minimal test case:

echo '<li><a></a><a></a></li>' | pup -v 'li a'

Should delete both <a> tags, but actually only deletes one. I suspect some sort of issue with my equality comparison, but I'm still working on figuring it out.

@wolfgang42
Copy link
Author

Closing as I couldn't figure out what was wrong with my code and no longer need to do this for the project I was working on anyway.

@wolfgang42 wolfgang42 closed this Dec 7, 2017
@Matheus28
Copy link

Matheus28 commented Dec 20, 2017 •
edited

It seems that once you remove the node in line 90: root.RemoveChild(node), the next iteration is gonna call node.NextSibling on that orphaned node, thus ending the loop for that subtree prematurely.

This is a good feature, I think it'd be a good idea to get the PR reopened.

@wolfgang42
Update tests for json{}
ddf33ef 
On the assumption that this is currently correct
but the tests were never updated after the formatter changed.

Closes ericchiang#80.
@wolfgang42
Modify tests to support multiple arguments.
3b0097e 
Also, update tests for --number to actually check that flag.
Previously they were checking for a '-n' tag inside an 'li' tag,
producing the empty string.
I don't think this was the intended result.
@wolfgang42
Implement --invert to remove tags matching selector.
be5dee7
@wolfgang42
Copy link
Author

@Matheus28 Thanks for the hint, I really should have noticed that I was mutating the list while I was iterating over it. I've updated the code to save a list of nodes to be removed and then remove them once they've all been found.

@wolfgang42 wolfgang42 reopened this Feb 17, 2018
@wolfgang42 wolfgang42 changed the title WIP: Implement --invert to remove tags matching selector. Implement --invert to remove tags matching selector. Feb 17, 2018
@wolfgang42
Copy link
Author

Also, I added tests. In the process I also updated some existing tests which seemed to be incorrect. See commit messages for details.

@eigengrau
Copy link

Would be great to see this merged!

@splitbrain
Copy link

This patch worked fine for me and should really be integrated.

@wolfgang42 wolfgang42 mentioned this pull request Sep 27, 2019
Open
@Aeres-u99
Copy link

Why is it not merged yet? its really something very useful imho.

@mre mre mentioned this pull request Nov 30, 2021
Closed
@frioux
Copy link

frioux commented Jan 24, 2022

I merged this into https://github.com/frioux/pup, fyi

@Aeres-u99
Copy link

Truly thanks \o/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature Request: strip tags that match a selector
6 participants
@wolfgang42 @Matheus28 @eigengrau @splitbrain @Aeres-u99 @frioux