-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Define available page sizes in backend class #57
Conversation
Reviewer's Guide by SourceryThe pull request defines available page sizes in the backend class. This change involves moving the Updated class diagram for PictureShow and ReportlabBackendclassDiagram
class PictureShow {
- _pic_files
- _backend
+ __init__(*pic_files)
+ save_pdf()
+ add_pictures()
+ _validate_target_path()
+ _validate_page_size()
}
class ReportlabBackend {
+ page_sizes
read_errors
}
PictureShow -- ReportlabBackend : has a
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 @mportesdev - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using a property instead of a static/class method for
_validate_page_size
. - It might be cleaner to define
page_sizes
as a dictionary instead of a tuple inReportlabBackend
.
Here's what I looked at during the review
- 🟡 General issues: 1 issue 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.
src/pictureshow/core.py
Outdated
|
||
class PictureShow: | ||
_page_sizes = dict(_backend_class.page_sizes) |
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.
suggestion: Consider the naming of _page_sizes for external use.
Since _page_sizes is referenced outside of the class, such as in the CLI, you might consider renaming it to a public name (e.g., page_sizes) or providing a public accessor to avoid relying on an underscore-prefixed attribute.
Suggested implementation:
class PictureShow:
page_sizes = dict(_backend_class.page_sizes)
Ensure that any external code referencing PictureShow._page_sizes is updated to reference PictureShow.page_sizes instead.
a420d60
to
cf09ccf
Compare
Summary by Sourcery
Define available page sizes in the backend class to allow for easier customization and to avoid a global constant.