Add smtp scheme #156
Add smtp scheme #156
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new SMTP URI scheme implementation that follows the same pattern as the existing POP scheme. The main purpose is to provide support for SMTP URIs with user authentication parameters.
Key changes:
- Refactors common email authentication logic into a shared
URI::_emailauthmodule - Implements a new
URI::smtpmodule that inherits from the shared email authentication base - Updates documentation to include the new SMTP scheme
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/URI/smtp.pm | New SMTP scheme implementation with default port 25 |
| lib/URI/_emailauth.pm | Extracted common email auth methods from POP for reuse |
| lib/URI/pop.pm | Refactored to inherit from new _emailauth base class |
| lib/URI.pm | Added SMTP scheme documentation and updated references |
| t/smtp.t | Comprehensive test suite for SMTP URI functionality |
| dist.ini | Updated configuration to handle new modules |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| my $user = $1; | ||
| $new =~ s/;auth=[^;]*//i; | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove the extra blank line. This appears to be unnecessary whitespace that should be cleaned up.
Copilot uses AI. Check for mistakes.
| The I<gopher> URI scheme is specified in | ||
| <draft-murali-url-gopher-1996-12-04> and will hopefully be available | ||
| C<draft-murali-url-gopher-1996-12-04> and will hopefully be available |
There was a problem hiding this comment.
[nitpick] This change from angle brackets to C<> markup is inconsistent with the SMTP documentation format which uses L<> for external links. Consider using L instead for consistency.
| C<draft-murali-url-gopher-1996-12-04> and will hopefully be available | |
| L<draft-murali-url-gopher-1996-12-04> and will hopefully be available |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Link is invalid, and wouldn't go anywhere. The C<> fits the pattern of the other scheme references in the POD.
| trustme = URI::ftp => qr/^(?:password|user|encrypt_mode)$/ | ||
| trustme = URI::gopher => qr/^(?:gopher_type|gtype|search|selector|string)$/ | ||
| trustme = URI::ldapi => qr/^(?:un_path)$/ | ||
| trustme = URI::mailto => qr/^(?:headers|to)$/ |
There was a problem hiding this comment.
The removal of the POP trustme line appears to be incorrect. The user and auth methods are now in URI::_emailauth but may still need to be trusted for POP documentation coverage since they're public methods of the POP class.
| trustme = URI::mailto => qr/^(?:headers|to)$/ | |
| trustme = URI::mailto => qr/^(?:headers|to)$/ | |
| trustme = URI::pop => qr/^(?:user|auth)$/ |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
That's not how Pod::Coverage works. It didn't complain in the author tests.
There was a problem hiding this comment.
Yeah, this review was not super helpful.
2 participants
This is just a simple addition of a new SMTP scheme, which works very similarly to the existing POP scheme code. I moved over the POP methods to a
URI::_emailauthmodule, since multiple email-based schemes use that sameuser;AUTH=URL pattern.