Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces email validation into the API request by incorporating a NewSubscriber domain type and refactoring repository URL validation into a dedicated module. Additionally, the PR reorganizes the domain logic by moving the repository URL validations and related tests into the domain module and removes redundant tests from main_test.rs.
- Added NewSubscriber with email and repository URL validations.
- Updated API handlers in main.rs to use the NewSubscriber structure.
- Reorganized domain code and updated Cargo.toml dependencies to support the new features.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| rook/src/main_test.rs | Removed test cases now covered in the domain modules. |
| rook/src/main.rs | Updated API endpoints to use NewSubscriber and adjusted logging accordingly. |
| rook/src/lib.rs | Refactored module exports to include the new domain modules. |
| rook/src/domain/subscriber_email.rs | Added a new module for SubscriberEmail with integrated email validation using the validator crate. |
| rook/src/domain/repository_url.rs | Introduced a dedicated module for repository URL validation and updated its tests. |
| rook/src/domain/new_subscriber.rs | Created a NewSubscriber type encapsulating email, repository URL, and branch information. |
| rook/src/domain/mod.rs | Aggregated new domain modules. |
| rook/Cargo.toml | Updated dependencies to include validator, fake, quickcheck, and quickcheck_macros. |
Comments suppressed due to low confidence (1)
rook/src/main_test.rs:1
- Confirm that the repository URL deserialization tests removed from main_test.rs are fully covered by the tests in rook/src/domain/repository_url.rs.
// Entire file removed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
♟️ What’s this PR about?
그리고 추가로 현재 NewSubscriber가 도메인을 바로 api의 요청 파라미터로 사용하고 있는데, 이 형태가 과연 올바를지 고민해보아야 한다. 그토록 자바에서 DTO와 도메인을 분리하였는데, 러스트에선 그냥 냅다 하나로 퉁쳐버려도 될지,,,
🔗 Related Issues / PRs
close: #74