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 example code in README #393

Closed
wants to merge 1 commit into from
Closed

Conversation

ppworks
Copy link
Contributor

@ppworks ppworks commented Feb 1, 2024

An error will occur if new is added to the class specified for text_filters.

Reading the source code, it appears that specifying the class is the correct behavior, so I changed the README.

Example

require 'html_pipeline'
require 'html_pipeline/text_filter/image_filter'
pipeline = HTMLPipeline.new(text_filters: [HTMLPipeline::TextFilter::ImageFilter.new])
lib/html_pipeline/text_filter.rb:7:in `initialize': wrong number of arguments (given 0, expected 1) (ArgumentError)

@gjtorikian
Copy link
Owner

I think actually the README is (aspirationaly) correct, and the code might be wrong. The other filters (node_filters) do expect an object, so I am not sure why I had it such that a class needs to be passed here. 🤔

@ppworks
Copy link
Contributor Author

ppworks commented Feb 2, 2024

Yes I too think that assuming an object is natural.

However, in the current code, if I pass an object by new method, I get an error.

lib/html_pipeline/text_filter.rb:7:in initialize': wrong number of arguments (given 0, expected 1) (ArgumentError)`

Maybe we should fix the code instead of the README 😄

@gjtorikian
Copy link
Owner

Maybe we should fix the code instead of the README

Yep--I will do this when I get the chance! Thank you for noticing.

@gjtorikian
Copy link
Owner

#398 will address this. Thanks for your PR and bringing this to my attention!

@gjtorikian gjtorikian closed this Feb 28, 2024
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.

2 participants