Refactor code with service to improve readability and testing, simplify msg

[soywod]

• Replace error-chain by anyhow [c619f06] Use state-of-the-art error library #152 Error handling refactoring. TornaxO7/himalaya#16
• Turn SMTP module into a service [5a94819]
• Refactor Config and Account management + repository Discussion about mail_refactor TornaxO7/himalaya#9
• Turn IMAP module into a service
• Clean folder structure
• Fix vim plugin

[TornaxO7]

May I ask you what you mean with "service" in your list above?

[soywod]

I will explain well this in the code documentation. A service is struct that implements a shared interface. Let's take the example of the SMTP service. There is a trait SmtpServiceInterface defined as:

pub trait SmtpServiceInterface {
    fn send(&mut self, msg: &lettre::Message) -> Result<()>;
}

Functions that need the service accept in parameter a SmtpServiceInterface. This way we will be able to test thoses functions by giving a fake service implementing this trait.

[soywod]

Basically, the main structure is on master (I'm quite satisfied of the result, let's see the benefits of it when we will add new tests). I still need to fix some part that I commented, to fix existing tests then we can start testing the rest (#198).

[TornaxO7]

Nice!

[soywod]

It seems that our last merge broke more stuff than I thought, I discovered a lot of tiny issues I'm fixing over time. Also, the Vim plugin is totally broken, since the JSON API of the Msg struct changed. My new objective is to fix as much as I can to have the same working state as before, to release a new version v0.5.0 (because of breaking changes) and then we will be able to add tests.

[TornaxO7]

Oh... sorry about this ._.

[soywod]

Oh no no, don't be sorry, I'm as faulty a you because I also approved and merge the branch. I should have tested more before merging. I let you know when the code will be ready to be tested 😉

[soywod]

I had a long thought last night on the code (especially about the Msg part). Here what came out (please feel free to challenge it):

• I'm not convinced anymore about one big Msg doing everything (notice that I do not blame you, I was as thrilled as you about your PR). Commands do not have the same need. For example, the list and search command do not need the half of what proposes Msg. The write, reply, forward commands do not need a Msg. They just need a template. Then the modified template is just piped to mailparse and lettre (still no need of Msg). Having just what we need for each commands could be a better approach IMO (less logic, simpler, easier to test and read). Maybe we will not even need a Msg struct?!
• While browsing Msg and playing with it, I realized that at lot of things can be reused else where instead of coding them. For example Headers: the lettre crate already implemented them, we could use them. Attachments also. The letter crate also has a message builder, we could use it instead of have our own?
• Code in comments are useless in fact, since himalaya is not a lib. I do not really need to know how to use the change_to_reply method, because the only implementation you will find is in himalaya itself. I do not know if you see what I mean.

Beside all this:

• I like the use of try_from
• I also like change_to_(reply/forward) method. It is a nice concept to start from a message and end with a reply or forward one.
• You put a lot of comments, links and tests. I take inspiration from them!

Let me know.

[TornaxO7]

I'm not convinced anymore about one big Msg doing everything (notice that I do not blame you, I was as thrilled as you about your PR). Commands do not have the same need. For example, the list and search command do not need the half of what proposes Msg. The write, reply, forward commands do not need a Msg. They just need a template. Then the modified template is just piped to mailparse and lettre (still no need of Msg). Having just what we need for each commands could be a better approach IMO (less logic, simpler, easier to test and read). Maybe we will not even need a Msg struct?!

Hm... I can understand what you mean. I'm a little bit irritated why you're still using an additional struct for creating templates? I thought that you would also use Msg for creating templates. The main idea of Msg was just to be able to become independent of imap. Otherwise you can only access the values as of the imap connection as long the connection is still on. If we're reverting this, we won't be able to "store" the values after closing the connection which would slow down the TUI pretty much, since you'd have to fetch the mail for each single action (listing the mails, viewing a single mail, downloading an attachment, ...).

While browsing Msg and playing with it, I realized that at lot of things can be reused else where instead of coding them. For example Headers: the lettre crate already implemented them, we could use them. Attachments also. The letter crate also has a message builder, we could use it instead of have our own?

Well yes, lettre already implemented them, but:

• lettre::address::Envelope includes only the To: and From: headers.
• lettre::message::Attachment is only used for building a message. You can't access the content of an attachment.

But we could "fix" those issues by applying more PRs to them (or I'm just blind and I pointed to the wrong structures...).

Code in comments are useless in fact, since himalaya is not a lib. I do not really need to know how to use the change_to_reply method, because the only implementation you will find is in himalaya itself. I do not know if you see what I mean.

Well, I had to experience in my work placement as well, that comments are seen as useless as well... so it's fine for me, if we reduce them.

[soywod]

The main idea of Msg was just to be able to become independent of imap.

It is exactly what the service refactor is for 😃 btw, the session is kept in memory. If I move the sess.logout() out of handlers (and put it in the main) you should be able to keep the session and prevent new logins!

Otherwise you can only access the values as of the imap connection as long the connection is still on.

First, I don't see how it prevents you to keep a local cache in the TUI code (let's say a HashMap<Uid, Message>). When the user requests messages, you could pick the ones available in the cache and fetch the missing ones. Secondly, I discussed about developing a lib/CLI about IMAP sync, in fact the Msg struct makes more sense there rather than in this himalaya CLI.

What I suggest is to implement a cache within the TUI code. When the v1 of himalaya will be ready, I will be able to start the development of this new CLI/lib to sync IMAP messages (you can join btw 😉), and we will be able to replace the cache by this lib.

Well yes, lettre already implemented them, but:

• lettre::address::Envelope includes only the To: and From: headers.
• lettre::message::Attachment is only used for building a message. You can't access the content of an attachment.

I was more talking about lettre::message::header rather than Envelope. For attachments you are right so we can skip. The only way to parse from a message is to use the mailparse crate.

Well, I had to experience in my work placement as well, that comments are seen as useless as well... so it's fine for me, if we reduce them.

Well, in a lib it is not useless at all. When the aim of your lib is to be used by other developers, examples are a must I would say. I'm not against, it's just not fitting a CLI.

[TornaxO7]

If I move the sess.logout() out of handlers (and put it in the main) you should be able to keep the session and prevent new logins!

I'm not such a fan of holding a connection for so long. For you cli-usage it's fine since each operation will take only some seconds but imagine someone else is using the TUI and let it open for hours. I think the server wouldn't be happy about this.

First, I don't see how it prevents you to keep a local cache in the TUI code (let's say a HashMap<Uid, Message>). When the user requests messages, you could pick the ones available in the cache and fetch the missing ones.

That's actually a neat idea! But I'd have to fetch everything again, if the user restarts the TUI 🤔

I will be able to start the development of this new CLI/lib to sync IMAP messages (you can join btw 😉),

This would be neat! I think that I'll join ^^

I was more talking about lettre::message::header rather than Envelope.

Oh I see, it looks pretty nice. I think we can switch to this.

Well, in a lib it is not useless at all. When the aim of your lib is to be used by other developers, examples are a must I would say. I'm not against, it's just not fitting a CLI.

ok

[soywod]

I'm not such a fan of holding a connection for so long. For you cli-usage it's fine since each operation will take only some seconds but imagine someone else is using the TUI and let it open for hours. I think the server wouldn't be happy about this.

Well, how do you think mail clients do? I would not be surprised if they keep the connection open. Thunderbird is not reconnecting each time I do an action, so they must keep the session somehow. In fact IMAP session are timed out, so it's not a problem to keep it open until it times out. If the user does an action after a while, we just need to check if the connection is still alive. If not then reconnect and go 😃

But I'd have to fetch everything again, if the user restarts the TUI

You can save state in files. It is what I'm gonna do for the IMAP sync lib/CLI.

[TornaxO7]

You can save state in files. It is what I'm gonna do for the IMAP sync lib/CLI.

Wouldn't it be better to use a database due to performance reasons?

[soywod]

Wouldn't it be better to use a database due to performance reasons?

Well, first it is a temporary solution the time for us to develop the IMAP sync lib. Secondly, I don't think we will have perf issue. You will parse it only once when you start the TUI so it's fine!

[soywod]

Hey @TornaxO7, I have some news (finally... I had hard times at work and I was not able to keep some free time for the tool):

• The tool and the vim plugin have been repaired 🔧
• The module structure has been improved 🌟
  • All the logic related to the domain has been moved to a domain folder
  • *_arg modules contain code related to the CLI and should only deal with CLI logic (nothing else, to avoid side effects)
  • *_entity modules contain models (Msg, Mbox etc)
  • *_service modules contain services (Smtp, Imap, Output)
  • *_handler modules are controllers, they contain all the logic
• The Msg struct has been simplified. I kept many logic from your PR, but many things changed (Headers have been merged with Msg, Attachments and Body have been merged into Part etc)

I propose the following:

• master is now stable enough (CLI + vim). I will release a new version v0.5.0 and I will not touch master anymore. Master will be the production branch (it should reflect the last release), and we will develop on another branch development. This way we will avoid compatibility problems between the CLI and the Vim plugin (since this last one always get the code from master and may be not up-to-date with the last release).
• I'm quite satisfied with the folder structure, but some parts of the code are not so pretty. I still need to clean + add comments.
• Once I'm satisfied with all the code, we will be able to add tests (if you still want to help). This refactor should really help us to write them (unit + integration).
• Once everything is good (code + comments + tests) we will be able to release a v0.6.0 and start cleaning issues till the v1.0.0
• Then there is the TUI and all other post-v1 features 💯

---

I really hope you will not take badly this refactor, since it touched a lot to your previous PR on the message refactor. I see your PR more like a "boost" that Himalaya needed to go forward. Many concept and ideas have been changed, but a lot have been kept also 😉

[TornaxO7]

I'm fine with this.

I'm a little bit busy at the moment due to my NintendoDS emulator which is my main project at this moment also my first semester just started and I'm pretty excited how it'll be :D

I'll try to help you with the other issues, if I'll find some time. Thank you for your time investment 👍

[soywod]

Thanks for the time you spent on Himalaya, you are so far the best contributor! Good luck with your project and semester 😃