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

Move sopel.web utilities into sopel.tools #1318

Closed
dgw opened this issue Apr 26, 2018 · 5 comments
Closed

Move sopel.web utilities into sopel.tools #1318

dgw opened this issue Apr 26, 2018 · 5 comments

Comments

@dgw
Copy link
Member

dgw commented Apr 26, 2018

Since sopel.web has been officially deprecated for over two years (and unofficially deprecated for even longer), I think it's time to consider what should actually be removed and what should be kept. The module as it currently exists in sopel.web should probably go away in Sopel 7, as we've had plenty of warning that it's deprecated. But some of its functions aren't marked @deprecated and do not seem to have direct replacements in requests or Python's stdlib.

Specifically, the quote, quote_query, iri_to_uri, and decode utility functions seem to have use cases still. I believe it would be best to put those where they seem like they belong: sopel.tools. That would get rid of sopel.web entirely, but preserve the bits that are not replaced by dropping requests in instead. There would, of course, be a transitional period (between the 6.x series and Sopel 7) during which the existing functions would be available in both namespaces.

The names of the new functions in sopel.tools are undecided, as I've just thought up this plan tonight. I'm not against having a sopel.tools.web subpackage, for example. I welcome input from module developers, who will be affected if staple functions for API interactions, like sopel.web.quote(), disappear.

Milestone target and details very much subject to change, as this is still in the conceptual/planning phase.

@dgw dgw added this to the 6.6.0 milestone Apr 26, 2018
@dgw dgw self-assigned this Apr 26, 2018
@freeboson
Copy link
Contributor

freeboson commented May 4, 2018

Also USER_AGENT/default_headers. I don't see what's wrong in principle of keeping these tools which are web-related within web. Even the functionality which is largely replaced by requests can still be neatly wrapped by functions in web.

@freeboson
Copy link
Contributor

sopel.web.quote is a good example of #1326. It's something that's useful for maintaining 2/3 compatibility, but it's already done by six, or could be simplified by dropping python2.

@dgw
Copy link
Member Author

dgw commented May 4, 2018

@freeboson Honestly, I go back and forth on whether to actually remove sopel.web at all. Under Embolalia's lead, the plans were made, and I started out just following through on the existing roadmap.

But I'll admit I'm devoting thought to a reverse-course. What I personally don't like about the get/head/post methods in sopel.web is the extremely limited subset of requests arguments they support. Turning sopel.web's wrappers into proper wrappers that let you pass any arbitrary kwarg to the underlying requests function, for advanced use, while automatically setting things like the user-agent for basic uses, would likely be what happens if I decide not to remove the module.

As far as moving things to sopel.tools goes, I just think it's weird that we have tools, and also web tools, but the web tools aren't inside the tools namespace. Does that make sense? To me, sopel.web sounds almost like a built-in HTTP server, which it isn't (and clearly so if you look at the API it provides, but still).

@dgw
Copy link
Member Author

dgw commented Mar 24, 2019

I started working on this this weekend. Not quite ready to open a PR for it just yet, but I did write up a plan in sopel-irc/sopel.chat@56a665d, copied here in case that commit eventually disappears (it's on a WIP branch that will probably get rebased several times before going live). The WIP code changes are also pushed here, for those curious enough to scan the branch list.

The tl;dr: Sopel 7 will have both sopel.web and sopel.tools.web. Stuff we're not keeping stays only in sopel.web; new stuff gets added only to sopel.tools.web. Renamed stuff has the old name in sopel.web and the new name in sopel.tools.web, but only one shared implementation/definition (in sopel.tools.web). Sopel 8 will do away with sopel.web entirely, keeping only sopel.tools.web.


While the whole sopel.web module was marked as deprecated in Sopel
6.2.0, because it largely serves as a wrapper around the requests
library, parts of it seem to be useful enough that they should be kept around.

For Sopel 8, we intend to move sopel.web to sopel.tools.web. Ideally the new
location will be available in Sopel 7 to provide a transitional period. Similar
to how importing from both willie and sopel worked in the run-up to Sopel
6.0, it will be possible to do any of the following during Sopel 7's life cycle:

  • import sopel.web
  • from sopel import web
  • import sopel.tools.web
  • from sopel.tools import web

In Sopel 8, we will remove the pointers from sopel.web to the new location.
These explicitly deprecated functions will also be removed at the same time:

  • sopel.web.get() — use requests.get() directly instead
  • sopel.web.head() — use requests.head() directly instead
  • sopel.web.post() — use requests.post() directly instead
  • sopel.web.get_urllib_object() — seriously, just use requests

We will also tweak the module constants:

  • sopel.web.default_headers: renamed to sopel.tools.web.DEFAULT_HEADERS
  • sopel.web.ca_certs: removed in sopel.tools.web — it no longer has any
    function (and was probably not useful for Sopel plugins to import, anyway)

New additions to Sopel's web tools (there may be a few) will be available only
in the new location (sopel.tools.web). Functions and constants that we plan to
remove (as listed above) will be available only from the old sopel.web module.

@dgw
Copy link
Member Author

dgw commented Aug 18, 2019

Should have been closed with merge of #1616, but someone (*cough* me) forgot to link things.

@dgw dgw closed this as completed Aug 18, 2019
@dgw dgw removed their assignment Aug 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants