Re-enable write_image with plotly 0.13 (incl features) #55
Conversation
WalkthroughThe changes update the project's dependencies and feature flags in the manifest, introducing a new optional dependency and several feature flags for static image export. In the code, conditional compilation enables a fully implemented Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Plot
participant PlotHelper
participant plotly_static
User->>Plot: write_image(path, width, height, scale)
activate Plot
Plot->>PlotHelper: get_image_format(path)
PlotHelper-->>Plot: ImageFormat or Error
alt Supported format
Plot->>plotly_static: write_image(path, width, height, scale, ImageFormat)
plotly_static-->>Plot: Result<(), Error>
else Unsupported or missing extension
Plot-->>User: Error
end
Plot-->>User: Result<(), Error>
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.toml(2 hunks)src/common/plot.rs(3 hunks)
🔇 Additional comments (8)
Cargo.toml (5)
3-3: Version bump looks appropriate for new functionality.The version increment from 0.9.5 to 0.9.6 is appropriate for adding new image export functionality.
21-21: Plotly dependency update aligns with PR objectives.The update to plotly 0.13 correctly addresses the backend change from kaleido to plotly_static as mentioned in the PR objectives.
22-22: Optional plotly_static dependency correctly configured.The plotly_static dependency is properly marked as optional, allowing users to opt into static export functionality only when needed.
27-31: Feature flag naming and organization follows good practices.The feature flags follow a clear naming convention and logically map to the underlying plotly crate features. The separation of different export backends (chromedriver, geckodriver, default) provides good flexibility for users.
30-30: Verify consistency of thestatic_export_wd_downloadfeature flagThe
static_export_wd_downloadfeature (Cargo.toml, line 30) only enablesstatic_export_wd_download = ["plotly/static_export_wd_download"]whereas the other static-export features also include
plotly_static. This discrepancy could lead to missing symbols or compilation errors if any code inplotly/static_export_wd_downloadexpectsplotly_static.Please confirm whether this omission is intentional. If
plotly_staticis required, update the feature entry:-static_export_wd_download = ["plotly/static_export_wd_download"] +static_export_wd_download = ["plotly/static_export_wd_download", "plotly_static"]src/common/plot.rs (3)
5-10: Conditional compilation correctly excludes static_export_wd_download.The conditional compilation attributes correctly exclude the
static_export_wd_downloadfeature since it doesn't include theplotly_staticdependency, which is consistent with the Cargo.toml configuration.
26-37: Method signature and conditional compilation look correct.The
write_imagemethod is properly gated by the relevant feature flags and has a reasonable signature for image export functionality. The return type using boxed error trait object provides good flexibility for error handling.
108-125: Image export implementation follows good patterns.The overall structure of the
write_imagemethod is well-designed:
- Properly creates a plotly instance and configures it
- Uses the helper trait for format detection
- Delegates to the underlying plotly implementation
- Returns appropriate error types
The implementation aligns well with the existing methods in the trait.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/common/plot.rs (1)
50-62: Great! The error handling suggestion has been implemented.The
get_image_formatmethod now properly returns aResulttype with descriptive error messages for unsupported formats, addressing the previous review feedback about replacing panic with proper error handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/common/plot.rs(3 hunks)
🔇 Additional comments (2)
src/common/plot.rs (2)
3-10: LGTM! Imports align with the new plotly_static backend.The conditional import of
ImageFormatfromplotly_staticcorrectly implements the feature flag approach described in the PR objectives.
26-37: LGTM! Well-structured method signature with proper feature gating.The
write_imagemethod declaration correctly uses conditional compilation and provides a flexible interface for image export with appropriate error handling.
| if let Some((filename, extension)) = path.into().rsplit_once('.') { | ||
| let format = self.get_image_format(extension)?; | ||
| plot.write_image(filename, format, width, height, scale)?; | ||
| } else { | ||
| Err("No extension provided for image.")?; | ||
| } |
There was a problem hiding this comment.
Improve error construction and consider edge cases in path handling.
While the error handling has been improved from the previous version, there are two issues to address:
- The error construction on line 126 is not idiomatic. Using
?on a string literal doesn't make sense. - The current logic might incorrectly handle paths with multiple dots (e.g.,
file.backup.pngwould usefile.backupas the filename without extension).
Apply this fix for proper error handling:
if let Some((filename, extension)) = path.into().rsplit_once('.') {
let format = self.get_image_format(extension)?;
plot.write_image(filename, format, width, height, scale)?;
} else {
- Err("No extension provided for image.")?;
+ return Err("No extension provided for image.".into());
}Consider using the full path including the extension when calling plot.write_image, as most image writing libraries expect the complete filename:
- if let Some((filename, extension)) = path.into().rsplit_once('.') {
+ let path_str = path.into();
+ if let Some((_, extension)) = path_str.rsplit_once('.') {
let format = self.get_image_format(extension)?;
- plot.write_image(filename, format, width, height, scale)?;
+ plot.write_image(&path_str, format, width, height, scale)?;
} else {
return Err("No extension provided for image.".into());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some((filename, extension)) = path.into().rsplit_once('.') { | |
| let format = self.get_image_format(extension)?; | |
| plot.write_image(filename, format, width, height, scale)?; | |
| } else { | |
| Err("No extension provided for image.")?; | |
| } | |
| let path_str = path.into(); | |
| if let Some((_, extension)) = path_str.rsplit_once('.') { | |
| let format = self.get_image_format(extension)?; | |
| plot.write_image(&path_str, format, width, height, scale)?; | |
| } else { | |
| return Err("No extension provided for image.".into()); | |
| } |
🤖 Prompt for AI Agents
In src/common/plot.rs around lines 122 to 127, the error handling uses `?` on a
string literal which is not idiomatic; replace this with a proper error type or
construct a custom error to return. Also, to handle filenames with multiple dots
correctly, avoid splitting the path at the last dot to separate filename and
extension; instead, use the full path including the extension when calling
`plot.write_image`. Adjust the logic to extract only the extension for format
detection but pass the entire filename with extension to the image writing
function.
This PR re-enables
write_imagethat was disabled due to #8. This comes from the update fromkaleidotoplotly_staticin plotly 0.13. Had to also bring inplotly_staticbecause of this bug.To manage the backend, I crated feature flags, in the same way that
plotlydid, described hereI also enabled (and tested) these image formats:
png,jpg,webpandsvg.Summary by CodeRabbit
New Features
Chores