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

Make torrequest (and other deps) optional #2130

Closed
2 tasks done
mjsir911 opened this issue May 21, 2024 · 3 comments · Fixed by #2215
Closed
2 tasks done

Make torrequest (and other deps) optional #2130

mjsir911 opened this issue May 21, 2024 · 3 comments · Fixed by #2215
Assignees
Labels
enhancement New feature or request

Comments

@mjsir911
Copy link
Contributor

mjsir911 commented May 21, 2024

Checklist

  • I'm reporting a feature request
  • I've checked for similar feature requests including closed ones

Description

Hi,

It would be cool if sherlock.py was modified a bit to not require the torrequest import if I have no interest in downloading via tor, preferably deferring to importing when it's actually needed or maybe wrapping in an exception handler and/or stubbing out.

Considering tor downloading won't work anyways without tor actually installed, shedding off an additional dependency (or two, I can't find stem being imported anywhere but it seems related) would be nice. The motivation behind this is torrequest and stem aren't currently packaged in gentoo's tree but all the other deps are.

In the same vein, exrex is only used within the tests but is pulled in the dependency list even for end-user installs. Moving this out to test-depends or something (I know python has this functionality, forget the name) would make sense.

@mjsir911 mjsir911 added the enhancement New feature or request label May 21, 2024
@ppfeister
Copy link
Member

torrequest wasn't available on Fedora / EPEL either. Seems like a dead project and I don't want to package something 6 years out of date. Here's my patch file that I'm using to remove it from my own build --

https://github.com/ppfeister/pkg/blob/master/sherlock/0001-Remove-tor.patch

Note that my patch is against the new package, #2111, which is pending code review

Removing features this way isn't ideal, but it's a fine short term fix in my opinion. I would like to figure something out for torrequest soon but that's my own fix for packaging in the meantime

I remember looking into removing stem but I don't remember why I didn't....... will have to investigate now. It may be a depend of another depend or something...

exrex has been removed from the dependency list in #2127, although it's very likely that I'll be adding it back for other tests. If I do, it'll be a dev depend and not in the default group. Should be gtg there either way.

@mjsir911
Copy link
Contributor Author

Removing features this way isn't ideal

Yeah, I'm interested in something much less destructive. Literally just moving the import would make things function until --tor is passed. Which is an acceptable tradeoff considering the app would fail anyways if tor isn't installed.

@ppfeister
Copy link
Member

Also, not sure if it was before or after your PR, but worth noting ----

I have actually removed the exrex dependency entirely with the shift to Poetry, tox, and pytest. So that specific issue is resolved.

@ppfeister ppfeister linked a pull request Jun 26, 2024 that will close this issue
@ppfeister ppfeister moved this from Backlog to In progress in Release 0.15.0 Jun 29, 2024
@ppfeister ppfeister removed a link to a pull request Jun 30, 2024
@ppfeister ppfeister self-assigned this Jul 1, 2024
@ppfeister ppfeister moved this from In progress to In review in Release 0.15.0 Jul 1, 2024
@ppfeister ppfeister moved this from In review to Approved in Release 0.15.0 Jul 1, 2024
@github-project-automation github-project-automation bot moved this from Approved to Done in Release 0.15.0 Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants