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

Jump to bottom

Proposal for fix_urllib #47566

Closed
nedds mannequin opened this issue Jul 7, 2008 · 13 comments
Closed

Proposal for fix_urllib #47566

nedds mannequin opened this issue Jul 7, 2008 · 13 comments
Labels
deferred-blocker topic-2to3

Comments

@nedds
Copy link
Mannequin

nedds mannequin commented Jul 7, 2008

BPO 3316
Nosy @warsaw, @brettcannon, @orsenthil, @benjaminp
Files
  • fix_urllib.diff: Diff for fix_urllib
  • fix_urllib.diff: Diff for updated fix_urllib
  • fix_urllib.diff: Diff for fix_urllib and test suite
    		•

    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 2008-07-16.05:13:47.214>
created_at = <Date 2008-07-07.20:36:53.430>
labels = ['deferred-blocker', 'expert-2to3']
title = 'Proposal for fix_urllib'
updated_at = <Date 2008-07-16.05:13:47.213>
user = 'https://bugs.python.org/nedds'

    bugs.python.org fields:

    activity = <Date 2008-07-16.05:13:47.213>
actor = 'brett.cannon'
assignee = 'collinwinter'
closed = True
closed_date = <Date 2008-07-16.05:13:47.214>
closer = 'brett.cannon'
components = ['2to3 (2.x to 3.x conversion tool)']
creation = <Date 2008-07-07.20:36:53.430>
creator = 'nedds'
dependencies = []
files = ['10845', '10846', '10903']
hgrepos = []
issue_num = 3316
keywords = ['patch']
message_count = 13.0
messages = ['69395', '69399', '69513', '69525', '69704', '69706', '69708', '69731', '69733', '69735', '69757', '69763', '69765']
nosy_count = 6.0
nosy_names = ['barry', 'brett.cannon', 'collinwinter', 'orsenthil', 'benjamin.peterson', 'nedds']
pr_nums = []
priority = 'deferred blocker'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue3316'
versions = []
    @nedds
    Copy link
    Mannequin Author

    nedds mannequin commented Jul 7, 2008

    Here is my proposed fix_urllib. The transform function is massive
    because there are a lot of cases, so maybe I should break it into
    separate functions for each case, and it could maybe do with some more
    documentation as well. I also use FromImport from fixer_util.py even
    though it warns against doing so for dotted imports because it appears
    to work. The temp.py file is a dummy python file that you can test it on
    to get an idea of what it does. I think it's got all the desired
    functionality, but I'm not an expert on the urllib changes so let me
    know if I missed anything. I haven't written tests for it yet, but
    assuming it looks reasonable, I can do that later today or tomorrow.

    @nedds nedds mannequin assigned collinwinter Jul 7, 2008
    @nedds nedds mannequin added the topic-2to3 label Jul 7, 2008
    @nedds
    Copy link
    Mannequin Author

    nedds mannequin commented Jul 7, 2008

    I broke up transform into the logical pieces of it, so I think now the
    code is a little bit more clear.

    @collinwinter
    Copy link
    Mannequin

    collinwinter mannequin commented Jul 10, 2008

    • You should add tests to test_fixers to ensure that this does what you
      expect. This will make it much easier for others to modify this fixer
      later. You can probably just reuse the tests Brett initially added.

    • There's a better data structure for MAPPING: rather than using

    MAPPING = {module: [(new_module, [member, member, ..., member])]}

    you should probably use

    MAPPING = {module: {new_module: [member, member, ..., member]}}

    The list-of-2-tuples was a bad idea on my part that didn't scale.

    • The "cannot handle star imports" warning isn't strictly correct: star
      imports for robotparser and urlparse can both be handled correctly,
      since they're just being renamed.

    • urlparse and robotparser don't seem to belong to this more specialized
      fixer: they can go in fix_imports, since they aren't being broken across
      multiple modules.

    @brettcannon
    Copy link
    Member

    What Collin said. =)

    I will put robotparser and urlparse into fix_imports myself and continue
    to update the docs in 2.x for urllib(2).

    Thanks to the both of you for helping with this! It's going to be great
    once this fixer is ready to go as it will put PEP-3108 REALLY close to
    being finished!

    @brettcannon
    Copy link
    Member

    Nick, are you going to be able to get this cleaned up today for the beta
    release tomorrow?

    @nedds
    Copy link
    Mannequin Author

    nedds mannequin commented Jul 15, 2008

    I should be able to. What time would you need it by?

    @benjaminp
    Copy link
    Contributor

    Today or tomorrow.

    @warsaw
    Copy link
    Member

    warsaw commented Jul 15, 2008

    I would really like to get this in by beta2, but it's not quite enough
    to hold up the release. Please try to get this cleaned up and committed
    by July 16 for beta 2.

    @nedds
    Copy link
    Mannequin Author

    nedds mannequin commented Jul 16, 2008

    I've got it finished, I just need to write some tests for it. It will
    all be done later tonight.

    @warsaw
    Copy link
    Member

    warsaw commented Jul 16, 2008

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA1

    On Jul 15, 2008, at 8:10 PM, Nick Edds wrote:

    Nick Edds <nedds@uchicago.edu> added the comment:

    I've got it finished, I just need to write some tests for it. It will
    all be done later tonight.

    Great!

    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.4.9 (Darwin)

    iQCVAwUBSH0/RHEjvBPtnXfVAQIdvAP/Sxz4XayNMlBWIicFVmmQ/C85Fujsb7wN
    tqznDWfM7rqCiChkSe2dMPoMTQsOntCIEze+d1Usc5SqqVQk45dSX/ARs2qVtI6g
    siVDSAxt2e1DWVCpko7hDySfP70e+1Id+Uv9obrHDJ6p6l7tx2hcZeS24NtQ+7aX
    vrCfAtAJZxk=
    =vtPk
    -----END PGP SIGNATURE-----

    @nedds
    Copy link
    Mannequin Author

    nedds mannequin commented Jul 16, 2008

    Here is a working version with tests. I believe it has all the desired
    functionality, but correct me if I'm wrong Brett. Also, I did not model
    MAPPING as suggested by Collin because order was important in generating
    tests, so I didn't think using a dictionary would be appropriate. As for
    star imports, right now they aren't supported, although I don't quite
    understand why they couldn't be. Also, if this looks good, could
    somebody commit it for me. I don't think I have write privileges. Sorry
    this took me until so late tonight to submit, I hadn't realized the high
    priority it had until this morning.

    @brettcannon
    Copy link
    Member

    I got your patch and it looks good overall. I made some style changes
    and upped the code reuse in some places. I am running the test suite now
    before I commit.

    @brettcannon
    Copy link
    Member

    r65002 has the patch. Thanks, Nick!

    @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
    Assignees
    No one assigned
    Labels
    deferred-blocker topic-2to3
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @brettcannon @warsaw @benjaminp