Skip to content

Conversation

ThomasLandauer
Copy link

Some questions/suggestions:

  1. Could you show an example for guzzleRequestOptions and/or deleteEmailsAfterScenario?
  2. I would merge "Added Methods" into "Example Test" (since it merely duplicates it). Is that OK?
  3. Why is $I->fetchEmails(); necessary? Why isn't is possible to call $I->openNextUnreadEmail(); directly?

Copy link
Owner

@koehnlein koehnlein left a comment

Choose a reason for hiding this comment

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

Hi Thomas, thanks for your input.

First of all I would like to explain, that this package is a fork of https://github.com/ericmartel/codeception-email-mailhog which was created for MailHog which is not maintained anymore.

My goal was to allow people to switch to Mailpit as easy as possible without any unnecessary changes.

Some questions/suggestions:

1. Could you show an example for `guzzleRequestOptions` and/or `deleteEmailsAfterScenario`?

To be honest, I never used these configurations by myself. But if you provide a pull request, I will check it will merge, if it is helpful.

2. I would merge "Added Methods" into "Example Test" (since it merely duplicates it). Is that OK?

I don't see a reason for that. The section "Added Methods" shows a short overview of the methods including a few words what te methods do, while "Example Test" is an example PHP code.

3. Why is `$I->fetchEmails();` necessary? Why isn't is possible to call `$I->openNextUnreadEmail();` directly?

This brings me back to the introduction I wrote at the top of the comment. This method always existed in the original package and my goal was to provide a 1:1 replacement as good as possible, so I never touched it. Of course it would be possible to refactor the functionality, but I do not see a reason to introduce such a major change.

README.md Outdated
composer require koehnlein/codeception-email-mailpit --dev
```
Then turn it on in your Codeception suite yaml file
Then enable it in your `Functional.suire.yml`:
Copy link
Owner

Choose a reason for hiding this comment

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

It's not necessarily required to use Functional.suite.yml. You could also use the module in acceptance tests and it's even possible to configure the module in the global codeception.yml

Copy link
Author

Choose a reason for hiding this comment

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

OK. But I think it's better to tell people the actual file name; this is also the way the Codeception docs are referring to those files.

Copy link
Owner

Choose a reason for hiding this comment

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

I checked https://codeception.com/docs/modules/Db as example and cannot confirm, it's done this way in the Codeception docs.

config:
Mailpit:
url: 'http://mailpit.dev'
url: 'http://localhost'
Copy link
Owner

Choose a reason for hiding this comment

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

It know that http://mailpit.dev is not a good example, because .dev is an existing TLD. But on the other hand, http://localhost is not really much better. I would prefer to have an example where everyone understands that it musst be changed to the real needs of the current setup, something like https://your-mailpit-instance or http://example.org

Copy link
Author

Choose a reason for hiding this comment

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

http://localhost is the default URL that Mailpit runs on. So this isn't meant as some placeholder-domain, but rather the actual URL that works for new users.
Here's what I have in my Functional.suite.yml:

    config:
        Mailpit:
            url: 'http://localhost'
            port: '8025'

With Composer:
```shell
composer req koehnlein/codeception-email-mailpit --dev
composer require koehnlein/codeception-email-mailpit --dev
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see a need to change this line. Why would you do it?

Copy link
Author

Choose a reason for hiding this comment

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

Just to explain what req stands for, cause some newbies might not understand it :-)

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 assume that users know how composer. The line is for simplification so that you can simply copy the command.

@ThomasLandauer
Copy link
Author

I don't see a reason for that. The section "Added Methods" shows a short overview of the methods including a few words what te methods do, while "Example Test" is an example PHP code.

Yeah, but it makes README unnecessarily long IMO. The code (with your nice comments) says it all.

This brings me back to the introduction I wrote at the top of the comment. This method always existed in the original package and my goal was to provide a 1:1 replacement as good as possible, so I never touched it. Of course it would be possible to refactor the functionality, but I do not see a reason to introduce such a major change.

Well, since MailHog is dead, you're the boss now! :-)
The reason for me suggesting it is that it's error-prone (and verbose) to always have to call some "internal" function manually. The function could be emptied out and marked @deprecated - then it wouldn't even be a breaking change.

BTW: I just suggested it to be prominently listed at https://codeception.com/addons - see Codeception/codeception.github.com#880

@koehnlein
Copy link
Owner

I don't see a reason for that. The section "Added Methods" shows a short overview of the methods including a few words what te methods do, while "Example Test" is an example PHP code.

Yeah, but it makes README unnecessarily long IMO. The code (with your nice comments) says it all.

This brings me back to the introduction I wrote at the top of the comment. This method always existed in the original package and my goal was to provide a 1:1 replacement as good as possible, so I never touched it. Of course it would be possible to refactor the functionality, but I do not see a reason to introduce such a major change.

Well, since MailHog is dead, you're the boss now! :-) The reason for me suggesting it is that it's error-prone (and verbose) to always have to call some "internal" function manually. The function could be emptied out and marked @deprecated - then it wouldn't even be a breaking change.

I would prefer to discuss that in two separate issues and not use this pull request for that.

BTW: I just suggested it to be prominently listed at https://codeception.com/addons - see Codeception/codeception.github.com#880

Thanks.

@ThomasLandauer ThomasLandauer deleted the patch-2 branch December 18, 2024 20:29
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