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

email module should have a way to prepend and insert headers #62339

Open
kbandla mannequin opened this issue Jun 5, 2013 · 10 comments
Open

email module should have a way to prepend and insert headers #62339

kbandla mannequin opened this issue Jun 5, 2013 · 10 comments
Labels
stdlib Python modules in the Lib dir topic-email type-feature A feature request or enhancement

Comments

@kbandla
Copy link
Mannequin

kbandla mannequin commented Jun 5, 2013

BPO 18139
Nosy @warsaw, @bitdancer
Files
  • suryak.patch: insert_header() to insert header at the beginning
  • 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 = None
    created_at = <Date 2013-06-05.04:35:54.493>
    labels = ['type-feature', 'library', 'expert-email']
    title = 'email module should have a way to prepend and insert headers'
    updated_at = <Date 2016-08-22.15:32:58.051>
    user = 'https://bugs.python.org/kbandla'

    bugs.python.org fields:

    activity = <Date 2016-08-22.15:32:58.051>
    actor = 'barry'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)', 'email']
    creation = <Date 2013-06-05.04:35:54.493>
    creator = 'kbandla'
    dependencies = []
    files = ['34278']
    hgrepos = []
    issue_num = 18139
    keywords = ['patch']
    message_count = 10.0
    messages = ['190641', '190658', '190661', '190662', '212642', '212645', '212646', '212731', '273380', '273381']
    nosy_count = 4.0
    nosy_names = ['barry', 'r.david.murray', 'kbandla', 'suryak']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = None
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue18139'
    versions = ['Python 3.6']

    @kbandla
    Copy link
    Mannequin Author

    kbandla mannequin commented Jun 5, 2013

    This issue is in message.py in the email module. The add_header method should insert the new header on the top of the existing headers. The current function implementation appends the header at the end of the existing headers.

    @kbandla kbandla mannequin added type-bug An unexpected behavior, bug, or error stdlib Python modules in the Lib dir labels Jun 5, 2013
    @bitdancer
    Copy link
    Member

    This is by design. I agree, however, that there should be a way to do this when an application needs it, so I'm changing this to a feature request.

    @bitdancer bitdancer changed the title email module's add_header appends instead of inserting email module should have a way to prepend and insert headers Jun 5, 2013
    @bitdancer bitdancer added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Jun 5, 2013
    @warsaw
    Copy link
    Member

    warsaw commented Jun 5, 2013

    In a somewhat similar vein, replace_header() retains the original header order. Not quite what the OP wants, but useful.

    The problem I had originally with a position-aware method is getting the API right. I didn't want to add a position argument to add_header() (and still don't). So I think a position-aware API would have to be a different method, perhaps with an API somewhat like list.insert()? It doesn't look like OrderedDict provides any guidance here.

    @bitdancer
    Copy link
    Member

    My preliminary thought (and I haven't checked the code yet to make sure this will actually work), is that under the new email policies we are dealing with full-blown header objects, meaning that they can know the header name they represent. This should theoretically make it possible to have a 'create header' function to create a header object (there may be one already...it's been way too long since I've worked on this code) and then have:

    message[:0] = [my_new_header]

    do the right thing. As well as all the other list methods working as expected. We'd also want a 'find_header' method that takes the header name and returns the index of the header in the list.

    But, as I say, this is just a preliminary thought, it needs careful consideration before we decide to actually do something.

    @suryak
    Copy link
    Mannequin

    suryak mannequin commented Mar 3, 2014

    I've done the following changes:

    1. create insert_header(): Which places header at the beginning.
      For that I created set() method which actually takes an argument pos default to 0.

    2. setitem() uses set() (new method) method while fixing pos to end.

    3. _parse_values(), new method created to not duplicate the code in insert_header() and add_header() since both of them merely do same job.

    Please let me know your opinions on it.

    @bitdancer
    Copy link
    Member

    The patch is a reasonable effort, thanks. However, before we can really evaluate any code, we need to agree on an API.

    The Message object presents a hybrid dictionary/sequence API over the headers. In the Sequence API, we insert something into the list using 'insert(pos, object)`. dict has no 'insert' method, so we don't need to worry about an API clash there.

    So we could define the method insert_header to have the following signature:

      insert_header(pos, name, value)

    add_header is still required, because we can't use insert_header to append. (It would then be nice if it were named append_header...but ignore that for now).

    However, there is almost no other context in which one interacts with the header list via a header's index in the list of headers. Message does not support the 'index' method.

    An alternate API, then, might be something like:

        insert_header_after(existing_name, new_name, value)

    This would be analogous to replace_header.

    The trouble with this, and the trouble with defining a header_index, is that multiple headers can have the same name. Message's answer to this currently is to have both a 'get' method and a 'get_all' method. The former returns the first match, the latter all of them.

    The reason this matters, by the way, is that one of the motivations for insert_header in my mind is the ability to create a sensible flow of headers: have the routing and forwarding headers (received, resent-xxx, etc) headers be before the From/to/date, etc headers. But to do that, you need to be able to insert them *after* the other headers with the same name.

    You could make 'insert after last instance of name' the default behavior of insert_header_after, but then we sill don't have a way to insert a header in an arbitrary location.

    Another possibility is 'insert_header' combined with both 'header_index' and 'header_index_all'.

    I *think* I'm leaning toward the latter (with my biggest hesitation being that insert_header is pretty much the only place you can use the index information returned by the index methods), but this kind of API design issue is something we should run by the email-sig mailing list. Feel free to post something there yourself, otherwise I will do so after I finish the 'whatsnew' work.

    @bitdancer
    Copy link
    Member

    (Aside: I have no idea why I set the stage to needs patch. The API discussion is needed first!)

    @suryak
    Copy link
    Mannequin

    suryak mannequin commented Mar 4, 2014

    Without really making big the in the API, one way of providing a position sensitive methods to enable more organized headers is to have. Having an index based api probably needs more discussion and thought!

    1. add_header_before('existing-header-name', 'new-name', value)
    2. add_header_after('existing-header-name', 'new-name', value)

    The existing-header-name is again taken by first-found.

    If put all routing, forwarded headers, one can always use add_header_before('From'...).

    @warsaw
    Copy link
    Member

    warsaw commented Aug 22, 2016

    I've updated this to Python 3.6, but I don't know if there's time to design an API and implementation in the time left before beta 1. But a use case has come up, so I want to reboot this discussion (yes, it should go to email-sig too).

    Apparently ARC headers <http://arc-spec.org/\> requires a header order. In Mailman we have a GSoC student who is working on an implementation, and he's stuck because he needs to *prepend* ARC headers on the message, but the API gives him no direct way to do this.

    We do have all the API problems that RDM points out. I'm not going to suggest to the student that they use Message._headers. It seems like the only way to do what he needs is to .get() all the headers in order, then del them all, manipulate the list out-of-band, and then re-add them all in the original order with the ARC headers at the front. Yuck.

    @warsaw
    Copy link
    Member

    warsaw commented Aug 22, 2016

    I meant .items()

    @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
    stdlib Python modules in the Lib dir topic-email type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants