-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Mime Parsing #447
base: master
Are you sure you want to change the base?
Mime Parsing #447
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @bwhitn - My review is from a 'readbility' point of view: would someone reading through this code in the future find it sensible and easy to understand?
} | ||
dataObj.forEach(function(obj) { | ||
if (obj.body) { | ||
let name = obj.fields.filename ? obj.fields.filename : obj.fields.name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you test for filename but not for the fallback, name
. is name
guaranteed to exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the name test below now. This could be let name = obj.fields.filename || obj.fields.name
.
dataObj.forEach(function(obj) { | ||
if (obj.body) { | ||
let name = obj.fields.filename ? obj.fields.filename : obj.fields.name; | ||
const type = obj.fields.type ? obj.fields.type : "text/plain"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could become const type = obj.fields.type || "text/plain"
for brevity
/** | ||
* Preferred MIME encoding format mappings. | ||
*/ | ||
export const MIME_FORMAT = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would make me so happy to have all these values aligned
/** | ||
* Extract data from mimeObjects and return object array containing them. | ||
* extractData([["testa", "header", "subheader"], ["testb", "header"]]) would | ||
* returns an array of objects {fields: {testa: "somestringornull", testb: "somestringornull"}, header: "somestringornull", body: "somestringornull"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor typo here
* | ||
* @param {Object} mimeObj | ||
*/ | ||
static decodeMimeMessage(mimeObj) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the description here you say 'Attempts to..' - what is the consequence of it failing to decode? Does it just leave the mimeObj
's data encoded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see the _decodeMimeData
throws. Please can you add a @throws
statement to the function comment? Its helpful for reviewers like me now and in the future.
input = Utils.convertToByteArray(input, "base64"); | ||
break; | ||
case "quoted-printable": | ||
input = decodeQuotedPrintable(input); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need a default case here?
* @param {string} boundary | ||
* @return {string[]} | ||
*/ | ||
static *_splitMultipart(input, boundary) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this star? I've never seen that before
* @param {string} input | ||
* @returns {byteArray} | ||
*/ | ||
export function decodeQuotedPrintable(input) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function comment has no summary
* | ||
* | ||
*/ | ||
run(input, args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hilariously long, empty multiline comment 😆
*/ | ||
import TestRegister from "../TestRegister"; | ||
|
||
TestRegister.addTests([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to see some more tests here for both Decode Mime Encoded Words and Parse Mime
* @license Apache-2.0 | ||
*/ | ||
class Mime { | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed?
input = Utils.strToByteArray(cptable.utils.decode(MIME_FORMAT[charEnc.toLowerCase()], input)); | ||
} | ||
return input; | ||
} catch (err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can do catch without err parameter, at least underscore it?
*/ | ||
class DecodeMimeEncodedWords extends Operation { | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class name says the function of the constructor
This should add Mime Parsing to included Mime Encoded Word Parsing.
This attempts to decode mime reguardless if it is \r\n (correct newline) or \n (incorrect)
Both Base64 and QuotedPrintable is used for decode. Others can be added later but currently these are the supported ones. UUencodeding and BinHex appear to be common.