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

documentation bugs ? #1942

Closed
tomaszdrozdz opened this issue Oct 1, 2020 · 7 comments
Closed

documentation bugs ? #1942

tomaszdrozdz opened this issue Oct 1, 2020 · 7 comments

Comments

@tomaszdrozdz
Copy link
Contributor

tomaszdrozdz commented Oct 1, 2020

https://sanic.readthedocs.io/en/latest/sanic/static_files.html

chunk_size = 1  

means 1 Byte ???

If so then

chunk_size = 1024 * 1024 * 8

Shouldn't it be 8MiB ???
Not 8KB, not 8MB but just 8MiB

@ashleysommer
Copy link
Member

Yes, I think you're right. That is a mistake in the documentation.

@tomaszdrozdz
Copy link
Contributor Author

Also what I found in

sanic/exceptions.py:

in docstring for

def abort(status_code, message=None):
    """...
    :param message: The HTTP response body. Defaults to the messages in response.py for the given status code."""

But in code it is:

from sanic.helpers import STATUS_CODES

if message is None:
    message = STATUS_CODES.get(status_code)

@tomaszdrozdz tomaszdrozdz changed the title static route - large file - chunk size - documentation static route - large file - chunk size | exceptions.py - documentation Oct 2, 2020
@tomaszdrozdz
Copy link
Contributor Author

tomaszdrozdz commented Oct 2, 2020

And please correct me if I am wrong but:
https://sanic.readthedocs.io/en/latest/sanic/blueprints.html#blueprint-middleware

Using blueprints allows you to also register middleware globally.

But it does not seams so, and even next paragraph
https://sanic.readthedocs.io/en/latest/sanic/blueprints.html#blueprint-group-middleware
states something different:

'applied on Blueprint : bp1 Only'


And in code registering middleware for blueprint is done with register_named_middleware,
So this part of documentation could be corrected.

@tomaszdrozdz tomaszdrozdz changed the title static route - large file - chunk size | exceptions.py - documentation documentation bugs ? Oct 2, 2020
@tomaszdrozdz
Copy link
Contributor Author

Also we could write tiny paragraph for:
register_named_middleware

@tomaszdrozdz
Copy link
Contributor Author

And also in middlewares documentation in listeners section for listeners execution order:

The listeners are deconstructed in the reverse order of being constructed.

I think it would be more clear to describe it like that:

"before_server_start", "after_server_start" listeners are executed in order they were registered
but "before_server_stop", "after_server_stop" listeners are executed in reverse order.

@ashleysommer
Copy link
Member

ashleysommer commented Oct 8, 2020

@tomaszdrozdz
As you can see there is a clear problem we are facing that code changes over time, but the documentation doesn't.
Eg:

:param message: The HTTP response body. Defaults to the messages in response.py for the given status code."""

This behavior changed when the standard HTTP response messages were refactored into "sanic.helpers" but the docstring for abort() (and possibly others) was not updated.
Similarly:

Using blueprints allows you to also register middleware globally.

This paragraph in the documentation was not updated when "named" middleware was introduced, so now blueprint middleware is only executed on the routes in that blueprint.

Of course this is a bad thing, but it is a very difficult problem to solve. When a user creates a pull request, they are only concerned about implementing a feature or fixing a bug they've found, and possibly creating a unit test for it. It is difficult for a contributor to know every place in the code that references that, or every piece of existing documentation which will now be inaccurate due to their change.

Its something we will need to continue to consider going forward.

@tomaszdrozdz
Copy link
Contributor Author

I understand :)
I just wanted to share what I have found.
But we can work on that - I will make pull request with documentation corrections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants