Skip to content

Conversation

@deathgrindfreak
Copy link

As we talked about previously, this commit adds any additional multipart headers as an optional record type parameter.

buffer = decodeText(buffer, 'binary', charset);
boy.emit('field', fieldname, buffer, false, truncated, encoding, contype);

var hdrs = Object.keys(additionalHeaders).length ? additionalHeaders : null;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can either use a simple boolean that is set in the for loop to avoid Object.keys() or we could just always pass additionalHeaders. I'm fine with either way.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we could just pass header as-is. That would save us from having to do any additional work.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, an empty object might be nice there, since it would avoid two null checks I suppose. I think I'll just pass in the object if that's cool with you.

else
encoding = '7bit';

for (var h in header) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just iterate over Object.keys(header) to avoid traversing prototypes and the like.

encoding = '7bit';

for (var h in header) {
if (!isRequiredHeader(h)) additionalHeaders[h] = header[h][0];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not be forcing the first value all the time, in case someone is interested in seeing all of the duplicate header values. Maybe at the very least we could just return the first value if header[h].length === 1 otherwise use the array value. Also just always assigning to header[h] so it's always an array for consistency is fine by me too.

@deathgrindfreak
Copy link
Author

Sounds good to me, added the changes in the above commit.

@deathgrindfreak
Copy link
Author

Any idea if this will be merged any time in the future?

@codeimmortal
Copy link

@mscdex any update to add this additional header feature

@Martin-Luther
Copy link

Hello guys,
First of all, thanks for your amazing work.
I was wondering: why not emitting an object with all properties at once: both required and additional properties (headers) ?

If not, when Cooper's changes will be merged with the master ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants