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

Correct port number in web example #1526

Merged
merged 2 commits into from
Mar 22, 2021
Merged

Correct port number in web example #1526

merged 2 commits into from
Mar 22, 2021

Conversation

twm
Copy link
Contributor

@twm twm commented Feb 27, 2021 •
edited
Loading

The Twisted Web in 60 Seconds example "Serving Static Content From a Directory" states that these are equivalent:

from twisted.web.server import Site
from twisted.web.static import File
from twisted.internet import reactor, endpoints

resource = File('/tmp')
factory = Site(resource)
endpoint = endpoints.TCP4ServerEndpoint(reactor, 8888)
endpoint.listen(factory)
reactor.run()

twistd -n web --path /tmp

However, they aren't because the default port for the web plugin is 8080 rather than 8888. They should match.

Contributor Checklist:

adiroiban
adiroiban approved these changes Mar 1, 2021
View reviewed changes
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

All good. Only a minor comment. Thanks!

If it's not a big deal, my suggestion is to also as a review from twisted-contributors team via the GitHub mechanism

image

We can experiment with this notification type and see if it works better than Trac report 25 .

docs/web/howto/web-in-60/static-content.rst Show resolved Hide resolved
@glyph
Copy link
Member

glyph commented Mar 21, 2021

We can experiment with this notification type and see if it works better than Trac report 25 .

The problem with this is that external contributors can't get their work into the queue. I also do this submitting for review but relying on the github mechanisms rather than https://twisted.reviews massively privileges project members over external contributors.

That said I hope we have a bot that can address this soon.

@adiroiban
Copy link
Member

That said I hope we have a bot that can address this soon.

@glyph As soon as I am done with speeding up the Twisted CI jobs , fixing flaky tests and fixing coverage I will look at creating the bot.

There is a PR for review to fix the template
#1555

@glyph
Copy link
Member

glyph commented Mar 21, 2021

@glyph As soon as I am done with speeding up the Twisted CI jobs , fixing flaky tests and fixing coverage I will look at creating the bot.

There is a PR for review to fix the template
#1555

Amazing. Really looking forward to it. Thank you for all your work, Adi!

@twm twm force-pushed the 9659-web-port-example branch from ab30123 to f3a1c93 Compare March 22, 2021 04:55
@twm
Copy link
Contributor Author

twm commented Mar 22, 2021

Thank you for the review @adiroiban!

@twm twm merged commit 38d878d into trunk Mar 22, 2021
@twm twm deleted the 9659-web-port-example branch March 22, 2021 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adiroiban adiroiban adiroiban approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@twm @glyph @adiroiban