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 a bug on remove_block_titles() and remove_block_url() #969

Merged
merged 1 commit into from
Mar 14, 2023

Conversation

xatier
Copy link
Contributor

@xatier xatier commented Mar 14, 2023

Fix the exception AttributeError: 'Filter' object has no attribute 'block_url' introduced in this commit [1].

self.block_title and self.block_url were members of the Filter object[2], but not anymore after commit [1].

This bug can be reproduced with setting WHOOGLE_CONFIG_BLOCK_URL to a non-empty string.

[1] 10a15e0
[2] 284a810

Fix the exception `AttributeError: 'Filter' object has no attribute 'block_url'`
introduced in this commit [1].

`self.block_title` and `self.block_url` were members of the Filter
object[2], but not anymore after commit [1].

This bug can be reproduced with setting WHOOGLE_CONFIG_BLOCK_URL to a
non-empty string.

[1] benbusby@10a15e0
[2] benbusby@284a810
@xatier
Copy link
Contributor Author

xatier commented Mar 14, 2023

@benbusby please kindly take a look.
I'm actually surprised that no previous bug reports for this one, perhaps we should add WHOOGLE_CONFIG_BLOCK_URL and WHOOGLE_CONFIG_BLOCK_TITLE to README?

Copy link
Owner

@benbusby benbusby left a comment

Choose a reason for hiding this comment

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

Thanks for fixing. I can't say I'm surprised that nobody has noticed until now though -- I was already under the impression that this feature wasn't used by very many people.

Regarding adding it to the README, I'm a bit unsure since it might be better to just remove the feature altogether in a future release. Both of these configuration values require the user to input a regex to make them work, which I don't think is very user friendly. Also there's already support for blocking multiple websites using a comma separated list, which is a lot easier than writing a regex, especially for non-technical users.

Either way, if you want to add documentation for these to the README, feel free to open another PR. But I guess I'm leaning more towards just removing this functionality in the near future unless there's a compelling reason to keep it.

@benbusby benbusby merged commit b1e468f into benbusby:main Mar 14, 2023
xatier added a commit to xatier/whoogle-search that referenced this pull request Mar 14, 2023
* add `WHOOGLE_CONFIG_BLOCK_TITLE` and `WHOOGLE_CONFIG_BLOCK_URL`

* this feature was originally added in 284a810
  but remained undocumented.

Ref: conversations in benbusby#969
@xatier xatier mentioned this pull request Mar 14, 2023
@xatier
Copy link
Contributor Author

xatier commented Mar 14, 2023

Sounds good, I've opened #971 to update the README. Perhaps this feature was low usage due to the fact it was undocumented?

I'm aware the WHOOGLE_CONFIG_BLOCK setting, but I think the impl has some 🐛? search_string is not a regex? https://github.com/benbusby/whoogle-search/blob/main/app/filter.py#L205

>>> x = 'foo.com,bar.com'
>>> search_string = ' '.join(['-site:' + _ for _ in x.split(',')])
>>> search_string
'-site:foo.com -site:bar.com'

@benbusby
Copy link
Owner

Not a bug really -- in this case the regex can just be the -site:... strings themselves so that BeautifulSoup will find partial matches. If we were to only pass the -site:... strings without re.compile, it wouldn't return any content from the result page since there wouldn't be any exact matches.

benbusby pushed a commit that referenced this pull request Mar 15, 2023
* add `WHOOGLE_CONFIG_BLOCK_TITLE` and `WHOOGLE_CONFIG_BLOCK_URL`

* this feature was originally added in 284a810
  but remained undocumented.

Ref: conversations in #969
@xatier
Copy link
Contributor Author

xatier commented Mar 15, 2023

I see, thanks for the clarification!

crown0128 added a commit to crown0128/whoogle-search that referenced this pull request Aug 10, 2023
* add `WHOOGLE_CONFIG_BLOCK_TITLE` and `WHOOGLE_CONFIG_BLOCK_URL`

* this feature was originally added in 284a8102c85bdc69674952bc30a6cfacfb8b01ee
  but remained undocumented.

Ref: conversations in benbusby/whoogle-search#969
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