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

imap, doc: documented stuffs on imapsession #2

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

yorkie
Copy link
Contributor

@yorkie yorkie commented Jun 29, 2014

I documented some stuffs and now updates the TODO List:

  • Documentation of each method in IMAPSession.js, parameters and examples, returned data.
  • appendMessage flags and date parameters should be made more nodeJS friendly + they should be made optional through an option parameter.
  • fetchMessagesByNumber / fetchMessagesByUid option parameter should also be made more nodeJS friendly.
  • remove options parameter of copyMessages: use only UID.
  • add tests(what i added)

LGTY?


* port Number, the port number of IMAP server
* hostname String, the hostname of IMAP server
* option Object, you config the authenticate option here
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to document what option can contain.

Copy link
Member

Choose a reason for hiding this comment

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

before showing the example.

@dinhvh
Copy link
Member

dinhvh commented Jun 29, 2014

You should split README.md in two:

  • One to describe what is the project and show a quick example.
  • Documentation.md, which shows a more complete documentation.

@dinhvh
Copy link
Member

dinhvh commented Jun 29, 2014

Do you want me to merge it and make progress in an other pull request?

@yorkie
Copy link
Contributor Author

yorkie commented Jun 29, 2014

You should split README.md in two

Ok, thanks, good idea :)

Do you want me to merge it and make progress in an other pull request?

In this Pull Request, we should fix the whole TODO listed below IMO, so may i ask u merge this once all the TODO has been strike.

And I'd like to do your comments tomorrow :)

@yorkie
Copy link
Contributor Author

yorkie commented Jun 30, 2014

fixed something and still leaves few questions:

appendMessage flags and date parameters should be made more nodeJS friendly + they should be made optional through an option parameter.

Can I create a new class named: IMAPMessage and let appendMessage be called like:

var msg = new IMAPMessage();
msg.date = new Date();
msg.flags.push('\Seen');
// ...some sets to `msg`
session.appendMessage(folder, message);

fetchMessagesByNumber / fetchMessagesByUid option parameter should also be made more nodeJS friendly.

What i think the best way to write the option just passing the command directly, like:

session.fetchMessagesByUID(0, 1, 'HEADER.FIELDS (FROM TO SUBJECT DATE)');

and for fresh we provide a default value for options: ALL, then they don't care about the option should be, however if they need, see the RFC3501 to learn how to concat their fetch command.

How about this? I need your review again, @dinhviethoa
Thank you :)

@dinhvh
Copy link
Member

dinhvh commented Jul 1, 2014

IMAPMessage already stands for something else in mailcore2. It's the result of fetchMessage...().
I don't think it's a good idea to introduce a new data type for the purpose of appendMessage().

For fetch options, in mailcore2, there's a bunch of options in a binary mask:

        IMAPMessagesRequestKindUid           = 0, // This is the default and it's always fetched
        IMAPMessagesRequestKindFlags         = 1 << 0,
        IMAPMessagesRequestKindHeaders       = 1 << 1,
        IMAPMessagesRequestKindStructure     = 1 << 2,
        IMAPMessagesRequestKindInternalDate  = 1 << 3,
        IMAPMessagesRequestKindFullHeaders   = 1 << 4,
        IMAPMessagesRequestKindHeaderSubject = 1 << 5,
        IMAPMessagesRequestKindGmailLabels   = 1 << 6,
        IMAPMessagesRequestKindGmailMessageID = 1 << 7,
        IMAPMessagesRequestKindGmailThreadID  = 1 << 8,
        IMAPMessagesRequestKindExtraHeaders  = 1 << 9,
        IMAPMessagesRequestKindSize          = 1 << 10,

We could map them in JS to something like ['uid', 'fullheaders', 'gmail-labels'].

@yorkie
Copy link
Contributor Author

yorkie commented Jul 1, 2014

then how we handle the PEEK in this option?

@dinhvh
Copy link
Member

dinhvh commented Jul 1, 2014

I'd suggest to do always PEEK. And let people mark explicitly as read with a storeFlags() API.

@yorkie
Copy link
Contributor Author

yorkie commented Jul 1, 2014

How do u think the naming of the element of the option? I prefer use a-b for all, do you?

@yorkie
Copy link
Contributor Author

yorkie commented Jul 1, 2014

Ok, updated document for fetch functions, if it good to u, i will update it to source code :)

@yorkie
Copy link
Contributor Author

yorkie commented Jul 1, 2014

and delete(complete) 3rd task in the top of this conversation.

@dinhvh
Copy link
Member

dinhvh commented Jul 1, 2014

How do u think the naming of the element of the option? I prefer use a-b for all, do you?

Sounds good to me.


* folder String
* data String, rfc822 message data
* flags Array
Copy link
Member

Choose a reason for hiding this comment

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

Can we make flags optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep

On Wed, Jul 2, 2014 at 12:47 AM, Hoà V. DINH [email protected]
wrote:

In docs/API.md:

+* text
+
+Otherwise like RFC3501, you should assign the option to ALL, FAST or FULL to do a shortcut.
+
+#### imap.search(query, callback)
+
+* query String
+* callback Function
+
+The SEARCH command implementation.
+
+#### imap.appendMessage(folder, data, flags, callback)
+
+* folder String
+* data String, rfc822 message data
+* flags Array

Can we make flags optional?


Reply to this email directly or view it on GitHub
https://github.com/MailCore/mailcore.node/pull/2/files#r14415276.

Yorkie Neil
https://github.com/yorkie
+18600219033

@yorkie
Copy link
Contributor Author

yorkie commented Jul 1, 2014

Ok, done :-)


* folder String
* data String, rfc822 message data
* flags Array
Copy link
Member

Choose a reason for hiding this comment

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

Could you document or link to some possible values for flags?

* extra-headers
* size

Otherwise like RFC3501, you should assign the `option` to `ALL`, `FAST` or `FULL` to do a shortcut.
Copy link
Member

Choose a reason for hiding this comment

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

Remove this line. We don't want people to use ALL, FAST, FULL, etc.

Copy link
Member

Choose a reason for hiding this comment

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

You still need to document the return value of fetchMessages...() in the callback.

@yorkie
Copy link
Contributor Author

yorkie commented Jul 3, 2014

Ok, fix some comments :-)

* gmail-thread-id
* size

And the element of `option` could also be custom header string like `X-Mailer`, `X-Envelope-Sender`, `X-Mailing-List` and etc.
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't it be ['uid', 'flags', {'custom-headers': ['X-Mailer', 'X-Enveloper-Sender']}]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, i think it is too nested :(

Copy link
Member

Choose a reason for hiding this comment

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

how about: 'uid', 'flags', 'custom-header:X-Mailer', 'custom-header:X-Mailing-List'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the custom-header and X- prefix seems act a same actor? i'm not sure if there is a custom header that doesn't contain a X- prefix?

Copy link
Member

Choose a reason for hiding this comment

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

There are custom header that contain a X-Prefix. And some headers are not listed as part of the "headers" or "full-headers".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, there is a problem on implementation:

fetchAtt = [{type:Constants.FetchEnvelope}, {type:Constants.FetchBodySection, param:'1.2'}]

This is a line that you write at libetpan.node, and could you provide a way of add custom header fetch item?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for example, how do i add the x-mailer to the fetchAtt?

Copy link
Member

Choose a reason for hiding this comment

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

Have a look at _fetchTypeToString() in imapbase.js. You can use {type: Constants.FetchBodySection, section:'HEADER.FIELDS (x-mailer mailing-list)'}.
I think the documentation in comments is wrong. It should be section and not param.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, i see

@dinhvh
Copy link
Member

dinhvh commented Jul 6, 2014

Also, you should start implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants