Edgewall Software
Modify

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#12131 closed defect (fixed)

Transitive import in trac/web/main.py

Reported by: Tim Graham <timograham@…> Owned by: Ryan J Ollos
Priority: normal Milestone: 1.2
Component: general Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:

Fixed transitive import in trac/web/main.py and added direct import of io module, replacing use of StringIO with io.BytesIO.

API Changes:
Internal Changes:

Description

The linked block references StringIO but it's not imported (unless it's coming from one of the import * imports (is it a goal to remove those?).

https://github.com/edgewall/trac/blob/8eaafa64ad6555e80ad7b0f0cdec1f340ebd6146/trac/web/main.py#L257-L262

Attachments (0)

Change History (13)

in reply to:  description comment:1 by Ryan J Ollos, 5 years ago

Replying to Tim Graham <timograham@…>:

The linked block references StringIO but it's not imported (unless it's coming from one of the import * imports (is it a goal to remove those?).

The code can be activated by appending ?hdfdump=1 to the URL, which enables a developer to view the values of the template parameters. The acronyms hdf is historical as far as I know, a holdover from when Trac used ClearSilver as the templating engine.

I think both your observations are correct. We shouldn't use from trac.web.api import *, especially since trac.web.api doesn't define __all__, and StringIO should be explicitly imported in trac.web.main. I'll fix the latter now.

comment:2 by Ryan J Ollos, 5 years ago

Milestone: 1.0.8

[14184,14186] adds import of StringIO to trac.web.main. Merged to trunk in [14185] (and the log message should say 1.2dev rather than 1.1.7dev, I figured I would make that error before too long …).

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:3 by Ryan J Ollos, 5 years ago

Defining __all__ for trac.web.api results in this bit of ugliness.

  • trac/web/api.py

    diff --git a/trac/web/api.py b/trac/web/api.py
    index 5041e49..14dca0a 100644
    a b from trac.util.translation import _  
    3939from trac.web.href import Href
    4040from trac.web.wsgi import _FileWrapper
    4141
     42__all__ = ['IAuthenticator', 'IRequestHandler', 'is_valid_default_handler',
     43           'IRequestFilter', 'ITemplateStreamFilter', 'parse_arg_list',
     44           'arg_list_to_args', 'RequestDone', 'Cookie', 'Request']
     45
    4246
    4347class IAuthenticator(Interface):
    4448    """Extension point interface for components that can provide the name
    for code in [code for code in HTTP_STATUS if code >= 400]:  
    228232    setattr(sys.modules[__name__], exc_name,
    229233            HTTPException.subclass(exc_name, code))
    230234    _HTTPException_subclass_names.append(exc_name)
     235__all__ += _HTTPException_subclass_names
    231236del code, exc_name

We could break convention and put __all__ at the end of the file.

comment:4 by Ryan J Ollos, 5 years ago

Milestone: 1.0.81.0.9

comment:5 by Ryan J Ollos, 5 years ago

Milestone: 1.0.91.2

in reply to:  3 comment:6 by Ryan J Ollos, 5 years ago

Replying to rjollos:

We could break convention and put __all__ at the end of the file.

I'm thinking this is probably more clear.

  • trac/web/api.py

    diff --git a/trac/web/api.py b/trac/web/api.py
    index 5041e49..7be9e32 100644
    a b class Request(object):  
    840840        for cookie in cookies.splitlines():
    841841            self._outheaders.append(('Set-Cookie', cookie.strip()))
    842842
     843
     844__all__ = ['IAuthenticator', 'IRequestHandler', 'is_valid_default_handler',
     845           'IRequestFilter', 'ITemplateStreamFilter', 'parse_arg_list',
     846           'arg_list_to_args', 'RequestDone', 'Cookie', 'Request'] + \
     847          _HTTPException_subclass_names
     848
     849
    843850__no_apidoc__ = _HTTPException_subclass_names

comment:7 by Tim Graham <timograham@…>, 5 years ago

Personally, I wouldn't worry too much about defining __all__ and just eliminate import *.

comment:8 by Tim Graham <timograham@…>, 5 years ago

Summary: Possibly dead code in trac/web/main.pyTransitive import in trac/web/main.py

comment:9 by Ryan J Ollos, 5 years ago

Owner: set to Ryan J Ollos
Status: newassigned

comment:10 by Ryan J Ollos, 5 years ago

Resolution: fixed
Status: assignedclosed

Fixed in [14214]. Thanks Tim!

comment:11 by Ryan J Ollos, 5 years ago

Release Notes: modified (diff)

comment:12 by Ryan J Ollos, 5 years ago

Release Notes: modified (diff)

comment:13 by Jun Omae, 5 years ago

I got AttributeError: class StringIO has no attribute 'StringIO' with hdfdump=1 parameter, #12151. I dislike uses of star imports :<

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.