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

removed support for tor #2200

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

removed support for tor #2200

wants to merge 3 commits into from

Conversation

sdushantha
Copy link
Member

Support for Tor has been removed. If users want to use Tor, it can be done using --proxy

Copy link
Collaborator

@matheusfelipeog matheusfelipeog left a comment

Choose a reason for hiding this comment

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

Perhaps, instead of removing it completely like this, wouldn't it be better to start notifying in the terminal that this feature is deprecated and will be removed in a future version? Just a smoother process for removing the feature so as not to catch users by surprise.

@ppfeister
Copy link
Member

ppfeister commented Jun 26, 2024

@matheusfelipeog This is part of the backlog for the bump to 0.15.0 and deprecation would be in the release notes alongside other planned breaking changes (i.e. importable module name changed for parity with package)

Thoughts on sufficiency?

Note that users can still use --proxy to route requests through a tor proxy (for the very few users that may exist)

@ppfeister ppfeister linked an issue Jun 26, 2024 that may be closed by this pull request
2 tasks
@ppfeister
Copy link
Member

@sdushantha Was this just a simple application of the patch or are there additional changes on top? Linting seems to fail but that's minor enough

@matheusfelipeog
Copy link
Collaborator

@matheusfelipeog This is part of the backlog for the bump to 0.15.0 and deprecation would be in the release notes alongside other planned breaking changes (i.e. importable module name changed for parity with package)

Thoughts on sufficiency?

@ppfeister Yes, I understand that. I think I wasn't very clear about the idea; I'll improve it:

  1. In the next version, whether it is major, minor, or patch, we would add a small notification to be shown in the terminal whenever the user runs the command with the --tor or --unique_tor flag.
  2. This message will inform the user that the feature will be discontinued in the future and that we recommend using the --proxy flag for this case.
  3. In a future major update, we would remove all code related to these tor features.

We already do something similar to inform that there is a new update of Sherlock, for example.

This makes the transition smooth and doesn't surprise the users. And realistically, very few people read the release notes.

@matheusfelipeog
Copy link
Collaborator

... Linting seems to fail but that's minor enough

Apparently because of this:

sherlock/sherlock.py:205:5: F841 Local variable `underlying_request` is assigned to but never used

@sdushantha
Copy link
Member Author

sdushantha commented Jun 28, 2024

@sdushantha Was this just a simple application of the patch or are there additional changes on top? Linting seems to fail but that's minor enough

@ppfeister This was a simple application of the patch
As @matheusfelipeog has pointed out, the linting can be easily fixed. Will fix today

@ppfeister
Copy link
Member

@matheusfelipeog I follow what you're saying. I lean slightly towards the "remove it now" side of the fence for this specific flag, but I'm not opposed to keeping it with a deprecation warning.

One thing I'd change, however...

If keeping it around for another version, I think we should adjust the import behavior to make it optional, as described in #2130. Prompt on first --tor run to install torrequest if not found. Bit of a middle ground between the "I'll never use this" majority, the not wanting to depend on a 6 year stale package with security concerns, and the occasional user who may have it in use still.

Thoughts?

Not sure why it's not in my patch file, but I was removing via sed in my spec instead.
@ppfeister ppfeister added the staged - do not merge PR is staged for coordinated release label Jun 29, 2024
@matheusfelipeog
Copy link
Collaborator

One thing I'd change, however...

If keeping it around for another version, I think we should adjust the import behavior to make it optional, as described in #2130. Prompt on first --tor run to install torrequest if not found. Bit of a middle ground between the "I'll never use this" majority, the not wanting to depend on a 6 year stale package with security concerns, and the occasional user who may have it in use still.

@ppfeister I agree. We can set it as optional in the pyproject.toml:

[tool.poetry.dependencies]
torrequest = { version = "^0.1.0", optional = true }

[tool.poetry.extras]
torrequest = ["torrequest"]

Users can install with:

pip install 'sherlock-project[torrequest]'

The remaining handling should be done in the code to avoid ModuleNotFoundError exceptions.

@ppfeister ppfeister linked an issue Jun 30, 2024 that may be closed by this pull request
1 task
Copy link
Member

@ppfeister ppfeister left a comment

Choose a reason for hiding this comment

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

Changes approved for rel 0.16.0
PR assigned to new issue on 0.16.0 project board

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
staged - do not merge PR is staged for coordinated release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove torrequest dependency
4 participants