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

Last stuff #18

Merged
merged 11 commits into from
Aug 29, 2021
Merged

Last stuff #18

merged 11 commits into from
Aug 29, 2021

Conversation

TornaxO7
Copy link
Owner

  • Making the attributes of mboxes independent. We can store them now as well!
  • Added more docs
  • Added type-safety for flags
  • Expanded flags a bit
  • Added more tests
  • Added a short summary of the file-structure in the beginning of the doc.

Please take a careful look into the mbox/model.rs file @soywod ! This is the last PR as far as I can tell. If I find something else, then I can make a PR on your repository after its merge ;)

- Making the attributes of mboxes independent. We can store them now as well!
- Added more docs
- Added type-safety for flags
- Expanded flags a bit
- Added more tests
- Added a short summary of the file-structure in the beginning of the doc.
@TornaxO7 TornaxO7 added this to the Msg refactoring milestone Aug 29, 2021
@TornaxO7 TornaxO7 requested a review from soywod August 29, 2021 19:13
Fixing help command description.
Fixing the link to the mbox delimiter.
Fixing a little test issue
src/imap/model.rs Outdated Show resolved Hide resolved
/// }
/// ```
pub fn add_flags(&mut self, mbox: &str, uid_seq: &str, flags: Flags) -> Result<()> {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why adding a new line here? It does not really fit the global style of the code base.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Should be fixed now.

/// Remove the flags to the message by the given information. Take a look on the example above.
/// It's pretty similar.
pub fn remove_flags(&mut self, mbox: &str, uid_seq: &str, flags: Flags) -> Result<()> {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why adding a new line here? It does not really fit the global style of the code base.

/// }
/// ```
pub fn set_flags(&mut self, mbox: &str, uid_seq: &str, flags: Flags) -> Result<()> {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why adding a new line here? It does not really fit the global style of the code base.

#[derive(Debug)]
pub struct ImapConnector<'a> {
pub account: &'a Account,
pub sess: imap::Session<TlsStream<TcpStream>>,
}

impl<'a> ImapConnector<'a> {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why adding a new line here? It does not really fit the global style of the code base.

src/flag/model.rs Show resolved Hide resolved
@@ -73,6 +71,94 @@ impl ToString for Flags {
}
}

impl ToString for Flags {
fn to_string(&self) -> String {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why adding a new line here? It does not really fit the global style of the code base.

}

// remove the trailing whitespace with the comma
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about .trim_end_matches()?

Copy link
Owner Author

Choose a reason for hiding this comment

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

well it's just a single whitespace, so I thought that pop might suit here. The compiler knows that he needs to remove only one character instead of looking after more whitespaces. At least I think that the compiler will think like this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But what if one time there is no white space? Or if we have an empty String? The program will panic, where the trim_end_matches will not. I know it's a bit extreme, but the less panicking code we have the better we will be (in my opinion).

Copy link
Owner Author

@TornaxO7 TornaxO7 Aug 29, 2021

Choose a reason for hiding this comment

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

Hm... ok, hold o!, I'll fix that quickly in a quick PR. (sorry, missread this part... ._.)

- Removed bold + capital words for logout-doc
- Run format on each *.rs file
@TornaxO7
Copy link
Owner Author

@soywod I threw cargo fmt on the project. second review pls

@TornaxO7
Copy link
Owner Author

Ok, I found the issue for the tests! The account has the smtp server etc. set to None. Hm... I think a suitable test trait as before might be suitable to avoid duplicated code. (since imap_test and account itself need a real-case Account struct).

- Testing the return value of the flags struct as a string doesn't really work
  since it's a HashSet => Converted it into a Vec (in the test) to set the order
  as well.
- Fixed imap test by reverting the changes in the test.
Changed error output when creating an Imap-Connection. Should help debugging :)
@TornaxO7
Copy link
Owner Author

@soywod tests are working now! You can review it again! 👍

Copy link
Collaborator

@soywod soywod left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@TornaxO7 TornaxO7 merged commit a858802 into mail_refactor Aug 29, 2021
@TornaxO7 TornaxO7 deleted the general_refactor branch August 29, 2021 22:19
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