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.

Sign up for GitHub

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

Jump to bottom

Transition web -> tools.web #1616

Merged
merged 3 commits into from
Jul 27, 2019
Merged

Transition web -> tools.web #1616

merged 3 commits into from
Jul 27, 2019

Conversation

dgw
Copy link
Member

@dgw dgw commented May 19, 2019

This is not ready (both merge-conflicted and unfinished), but I'm shamelessly stealing PR number 1616 because it's a cool number. I should have opened a WIP PR for this weeks/months ago, anyway.

I've been talking about this for a while, and already wrote the migration guide section on it a while back. It's definitely happening… once I have a chance to rebase & finish this.

@dgw dgw added this to the 7.0.0 milestone May 19, 2019
@dgw dgw force-pushed the transition-sopel.web branch 2 times, most recently from 81ef252 to dea135a Compare June 2, 2019 19:14
@dgw dgw marked this pull request as ready for review June 2, 2019 19:22
@dgw dgw requested a review from a team June 2, 2019 19:32
@dgw
Copy link
Member Author

dgw commented Jun 2, 2019

The import updates are straightforward, touching as little as possible. Some of them could be streamlined, I'm sure (especially with @Exirel's righteous push toward making imports consistent, and using relative imports where appropriate), but I stuck with the lowest-impact changes possible. Refactoring stuff to call web tools differently is for another PR, IMO.

@half-duplex half-duplex mentioned this pull request Jun 4, 2019
Merged
@dgw
Copy link
Member Author

dgw commented Jul 6, 2019

New merge conflicts from #1578. I'll also need to rebase/rework this on top of #1582, because that PR limits what gets imported from sopel/web.py even though most of it is moving to sopel/tools/web.py.

@Exirel
Copy link
Contributor

Exirel commented Jul 9, 2019

Once you're good with the conflict, I'll add my review. As far as I can see, it should be all good to me.

@dgw dgw force-pushed the transition-sopel.web branch 2 times, most recently from 6f04dbf to 623c230 Compare July 26, 2019 12:35
@dgw
Copy link
Member Author

dgw commented Jul 26, 2019

We'll pretend that second force-push didn't happen. x)

@Exirel, when things clear up for you at work, I think this is ready now. 😸

dgw added 3 commits July 26, 2019 07:41
@dgw
web: move non-deprecated utilities to sopel.tools.web
75e7912 
The deprecated functions remain available only from `sopel.web`, to be
left behind as plugin authors transition their code to the new import
location. Nothing imported from `sopel.web` should break.

In the new `tools.web` module, both `USER_AGENT` and `DEFAULT_HEADERS`
should be all-caps, so they really do look like constants.

`web.ca_certs` appears to have been irrelevant for ages now. Left it in
the old import location just in case some silly module is using it for
something (don't know what; it'll always be `None` now), and it will go
away when `sopel.web` is deleted completely in Sopel 8.0..

Removed imports of `ssl` module and all the gymnastics about getting
`match_hostname` from backports if needed, since nothing in `web` calls
on the `ssl` module directly any more. Not even the wrapper functions.
@dgw
Update imports of sopel.web -> sopel.tools.web
d00168a
@dgw
test: update web -> tools.web; ignore deprecated package
ad27c8f
@dgw dgw force-pushed the transition-sopel.web branch from 623c230 to ad27c8f Compare July 26, 2019 12:44
Exirel
Exirel approved these changes Jul 26, 2019
View reviewed changes
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

Everything's looking good to me.

Is sopel.web part of the documentation, and if so, sopel.tools.web might want to be?

@dgw
Copy link
Member Author

dgw commented Jul 26, 2019

sopel.web is explicitly not part of the documentation since 6-point-something:

Note that sopel.web was deprecated in 6.2.0, and is not included in this documentation, but is still in use in many modules. — https://sopel.chat/docs/api.html as of 2019-07-26 / version 6.6.9

But that doesn't mean that sopel.tools.web shouldn't be included. Since we're keeping it, it probably should be documented. Tempted to do that in a separate PR, since I really don't want to keep this one open for too much longer and thus end up needing to resolve more merge conflicts! 😁

@Exirel
Copy link
Contributor

Exirel commented Jul 26, 2019

Tempted to do that in a separate PR, since I really don't want to keep this one open for too much longer and thus end up needing to resolve more merge conflicts! 😁

Suit yourself, I already approved the PR anyway! ;-)

@dgw dgw mentioned this pull request Jul 26, 2019
Merged
@dgw
Copy link
Member Author

dgw commented Jul 26, 2019

New PR done: #1669. This one is now on the short list of "Approved" PRs I can merge Soon™.

@dgw dgw merged commit 3b71a04 into master Jul 27, 2019
@dgw dgw deleted the transition-sopel.web branch July 27, 2019 20:04
This was referenced Aug 18, 2019
Closed
Closed
@dgw dgw mentioned this pull request Dec 1, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Exirel Exirel Exirel approved these changes

Assignees
No one assigned
Labels
Documentation Medium Priority Tweak
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@dgw @Exirel