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

add 'directory' option to the http.server module #72893

Closed
matrixise opened this issue Nov 15, 2016 · 11 comments
Closed

add 'directory' option to the http.server module #72893

matrixise opened this issue Nov 15, 2016 · 11 comments
Labels

Comments

@matrixise
Copy link
Member

@matrixise matrixise commented Nov 15, 2016

BPO 28707
Nosy @vstinner, @matrixise, @zhangyangyu, @JulienPalard
PRs
  • #1776
  • Files
  • chdir-httpserver.diff
  • 28707.patch
  • issue28707.diff
  • issue28707.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2017-05-24.07:32:22.284>
    created_at = <Date 2016-11-15.22:31:49.043>
    labels = ['3.7']
    title = "add 'directory' option to the http.server module"
    updated_at = <Date 2017-05-24.07:32:22.283>
    user = 'https://github.com/matrixise'

    bugs.python.org fields:

    activity = <Date 2017-05-24.07:32:22.283>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-05-24.07:32:22.284>
    closer = 'vstinner'
    components = []
    creation = <Date 2016-11-15.22:31:49.043>
    creator = 'matrixise'
    dependencies = []
    files = ['45493', '45494', '45753', '45764']
    hgrepos = []
    issue_num = 28707
    keywords = ['patch']
    message_count = 11.0
    messages = ['280898', '280899', '280903', '280906', '280916', '282346', '294292', '294293', '294311', '294332', '294334']
    nosy_count = 4.0
    nosy_names = ['vstinner', 'matrixise', 'xiang.zhang', 'mdk']
    pr_nums = ['1776']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue28707'
    versions = ['Python 3.6', 'Python 3.7']

    @matrixise
    Copy link
    Member Author

    @matrixise matrixise commented Nov 15, 2016

    When we execute the http.server module, the tool will use the current directory (os.getcwd()) but sometimes we would like to specify a directory on the command line.

    With the next patch, I try to fix this missing feature ;-)

    Just with python -m http.server -d /tmp

    by default the system will use the current directory.
    if necessary, I will show an error if the directory does not exist.

    @matrixise matrixise added the 3.7 label Nov 15, 2016
    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Nov 15, 2016

    The new feature should be documented in Doc/library/http.server.rst, and maybe also Doc/whatsnew/3.7.rst.

    Is it possible to write an unit test?

    @JulienPalard
    Copy link
    Member

    @JulienPalard JulienPalard commented Nov 15, 2016

    Hi Stéphane,

    Your patch is simple and elegant, but I'm asking myself a question about the idea to pass a class instead of an instance to the TCPServer ctor (I know that's not your choice).

    If we were able to pass an instance of SimpleHTTPRequestHandler instead of its class, we'd be able to give the directory to the handler during the main(), instead of using with chdir and getcwd to pass the information in a kind of hidden/side channel.

    I think that relying on chdir and getcwd to pass a parameter is not the most pretty or most testable way to do so. Also, having an os.getcwd() hardcoded in SimpleHTTPRequestHandler does not looks right (again, not your fault, it was here before…) typically for testability, but also when used in a concurrent environment, when cwd may be changed by other "coroutines".

    I tried to provide a patch fixing all those condiderations, here it is. still, I can't write a patch as KISS as yours, so I'm probably in the wrong way, at least I tried.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Nov 16, 2016

    Julien Palard added the comment:

    If we were able to pass an instance of SimpleHTTPRequestHandler instead of its class, we'd be able to give the directory to the handler during the main(), instead of using with chdir and getcwd to pass the information in a kind of hidden/side channel.

    You may be able to use functools.partial() to pass an additional
    parameter to the request handler constructor.

    We use something like that in asyncio for protocols.

    @zhangyangyu
    Copy link
    Member

    @zhangyangyu zhangyangyu commented Nov 16, 2016

    +1 for this idea. I use this command everyday in work and every time must cd to the directory is an annoyance.

    @JulienPalard
    Copy link
    Member

    @JulienPalard JulienPalard commented Dec 4, 2016

    @victor you're right, I forgot that partial can also apply keyword arguments. Here is a new patch.

    @matrixise
    Copy link
    Member Author

    @matrixise matrixise commented May 23, 2017

    Victor,

    I have added the documentation, but for the unit test, there is already a test with the SimpleHTTPRequestHandler class where they specify the cwd attribute, because the code of mdk does not change the behavior, I think it's not needed to write a test.

    now, if you think we need a test for this specific case (in fact we only override the constructor of the class and specify the directory parameter), I can write a test, but check the code before.

    Thank you.

    @matrixise
    Copy link
    Member Author

    @matrixise matrixise commented May 23, 2017

    and of course, sorry for this big delay.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented May 24, 2017

    Stéphane Wirtel added the comment:

    and of course, sorry for this big delay.

    I am not sure why you are saying that. Nobody was actively awaiting on this
    feature, we all have busy scedules. It's ok ;-)

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented May 24, 2017

    New changeset a17a2f5 by Victor Stinner (Stéphane Wirtel) in branch 'master':
    bpo-28707: Add the directory parameter to http.server.SimpleHTTPRequestHandler and http.server module (bpo-1776)
    a17a2f5

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented May 24, 2017

    Patch merged. Thanks Stéphane and Julien! Especially because writing the unit test was much more painful than I expected, but it's really worth it!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants