-
Notifications
You must be signed in to change notification settings - Fork 219
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
feat: validate Data and Message models on construction #1139
Conversation
Reviewer's Guide by SourceryThis PR implements validation for the Sequence diagram for JSON validation flowsequenceDiagram
participant Client
participant Data
participant Message
Client->>Data: fromJson(jsonMap)
activate Data
Data->>Data: Validate messages key exists
Data->>Data: Validate messages is list
Data->>Data: Validate messages not empty
loop For each message
Data->>Message: fromJson(messageJson)
activate Message
Message->>Message: Validate required keys
Message->>Message: Set default values
Message-->>Data: Return Message instance
deactivate Message
end
Data-->>Client: Return Data instance
deactivate Data
Class diagram showing updated Data and Message models with validationclassDiagram
class Data {
+List~Message~ messages
+fromJson(Map json)
note for Data "Added validation:
- messages key exists
- messages is a list
- messages not empty"
}
class Message {
+List~String~ text
+bool flash
+bool marquee
+Speed speed
+Mode mode
+fromJson(Map json)
note for Message "Added validation:
- text, speed, mode keys exist
- text is a list
- Default values for flash/marquee"
}
Data *-- Message : contains
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ZauberNerd - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using custom exception classes (e.g.,
InvalidMessageFormatException
) instead of genericException
for more specific error handling
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1dbfd8c
to
21c0adf
Compare
Hey @ZauberNerd, it would be best if you could raise PRs by adding the appropriate issue in them. If an issue is not present for the same, raise an issue for it, mentioning whether it is a bug or an enhancement, etc., and then raise a PR for it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome
This is a review suggestion by sourcery-ai. Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
This is a review suggestion by sourcery-ai. Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
b8034e3
to
3f67490
Compare
This PR implements validation of
Data
andMessage
inputs and sets sane defaults for missing values.Summary by Sourcery
Implement validation for
Data
andMessage
models during construction, setting default values for missing fields.New Features:
Data
andMessage
model inputs upon creation.Tests: