feat(engine): add sheet_name parameter to excel_write() #568
Conversation
✅ Deploy Preview for reearth-flow canceled.
|
soma00333
force-pushed
the
feat/engine/addparameter-to-excelwrite
branch
from
b716fbb to
741f85d
Compare
WalkthroughThe changes primarily involve enhancements to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🔇 Additional comments (5)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (2)
engine/runtime/examples/plateau/testdata/workflow/example_excel.yml (1)
Line range hint
1-41: Suggestions for enhancing the workflow exampleWhile the current implementation is correct, here are some suggestions to improve the example:
Consider expanding the sample data in the FeatureCreator to include more diverse entries or additional fields. This could help demonstrate the full capabilities of the Excel output.
It might be helpful to add a comment explaining the usage of
env.get("outputPath")for clarity, especially for new users.To showcase more complex workflows, you could consider adding intermediate nodes for data processing or validation between FeatureCreator and FileWriter.
Here's an example of how you might expand the FeatureCreator data:
creator: | [ #{ country: "Japan", city: "Tokyo", population: 37977000, area: 2194, density: 6158 }, #{ country: "Japan", city: "Osaka", population: 14977000, area: 2722, density: 4639 }, #{ country: "Japan", city: "Nagoya", population: 9552000, area: 5172, density: 2625 } ]And here's how you might add a comment for the output path:
output: | # outputPath is set in the environment and defines the directory for output files env.get("outputPath") + "sample.xlsx"These changes would make the example more comprehensive and easier to understand for users.
engine/schema/actions.json (1)
Line range hint
1363-1392: Consider similar enhancements for other formats and maintain consistency.While the addition of the
sheetNameproperty for Excel format is a good improvement, it's worth considering if similar flexibility could benefit other output formats. For example:
- For CSV/TSV: Consider adding an optional
headerRowproperty to allow customization of the first row.- For JSON: An optional
rootElementNamecould allow customization of the root JSON object name.Additionally, maintain consistency across formats where possible. For instance, if
converteris applicable to formats other than JSON, consider adding it to those formats as well.Would you like me to propose a more generalized schema structure that could accommodate format-specific options more flexibly?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
engine/docs/mdbook/src/action.mdis excluded by!**/*.mdengine/sample.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (4)
- engine/runtime/action-sink/src/file/excel.rs (1 hunks)
- engine/runtime/action-sink/src/file/writer.rs (4 hunks)
- engine/runtime/examples/plateau/testdata/workflow/example_excel.yml (1 hunks)
- engine/schema/actions.json (1 hunks)
🧰 Additional context used
🔇 Additional comments (8)
engine/schema/actions.json (2)
1383-1392: LGTM: NewsheetNameproperty added for Excel format in FileWriter.The addition of the
sheetNameproperty to the Excel format option in the FileWriter action is a good enhancement. It allows users to specify a custom sheet name when writing to Excel files, which increases flexibility and usability.A few observations:
- The
sheetNameproperty is correctly defined as optional (can be null).- It uses the
#/definitions/Exprreference, allowing for dynamic sheet naming through expressions.- The property is added only to the Excel format option, maintaining backwards compatibility for other formats.
Line range hint
1-2112: Overall, well-structured schema with appropriate enhancement.The
actions.jsonfile presents a comprehensive and well-organized schema for various data processing actions. The addition of thesheetNameproperty to the FileWriter action for Excel format is an appropriate enhancement that integrates seamlessly into the existing structure.Key points:
- The schema maintains a consistent structure across different actions.
- Each action is clearly defined with appropriate parameters, input/output ports, and categories.
- The new
sheetNameproperty adheres to the established pattern for optional properties.The change improves the functionality of the FileWriter action without disrupting the overall schema design or backwards compatibility.
engine/runtime/action-sink/src/file/writer.rs (4)
18-18: Add necessary import for ExcelWriterParamThe import of
ExcelWriterParamis essential for the new Excel functionality introduced.
113-114: Include Excel-specific properties in FileWriterParamAdding
excel_propertyto theExcelvariant ofFileWriterParamallows for Excel-specific parameters, aligning with the existing pattern used in theJsonvariant.
126-128: Update to_common_param method to handle Excel variantThe
to_common_parammethod now includes theExcelvariant, ensuring that the common properties are correctly returned for Excel file operations.
165-167: Handle Excel variant in finish methodThe
finishmethod is updated to handle theExcelvariant, invokingwrite_excelwith the appropriate parameters. This integration ensures that Excel files are written correctly using the new functionality.engine/runtime/action-sink/src/file/excel.rs (2)
16-20: Definition ofExcelWriterParamstruct is appropriateThe struct is correctly defined with the necessary derives for serialization, deserialization, debugging, cloning, and JSON schema generation. The
sheet_namefield is properly declared with the correct visibility.
31-36: Setting worksheet name withsheet_nameparameter works correctlyThe code appropriately retrieves the
sheet_namefromparams, defaulting to"Sheet1"if none is provided, and sets it usingset_name. This enhances the flexibility of the Excel writing functionality.
| @@ -33,6 +33,7 @@ graphs: | |||
| format: excel | |||
| output: | | |||
| env.get("outputPath") + "sample.xlsx" | |||
| sheetName: "SampleSheet" | |||
There was a problem hiding this comment.
💡 Codebase verification
Schema Missing sheetName Property
The JSON schema does not include the sheetName property for the FileWriter action. Please update the schema to include this property to ensure consistency and proper validation.
🔗 Analysis chain
LGTM: Sheet name property added correctly.
The sheetName property has been added to the FileWriter action with the value "SampleSheet". This change aligns with the PR objectives of allowing users to specify the name of the sheet when writing to an Excel file.
A few observations:
- The indentation is correct, maintaining the YAML structure.
- The property name
sheetNamematches the camelCase convention used in the rest of the file. - The value "SampleSheet" is appropriately quoted as a string.
To ensure this change is reflected in the schema, let's verify the JSON schema file:
If the result is true, it confirms that the schema has been updated to include the new sheetName property.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the JSON schema includes the new sheetName property
# Expected: The schema should include a sheetName property for the FileWriter action
jq '.definitions.FileWriter.properties.with.properties | keys | contains(["sheetName"])' < <(curl -s https://raw.githubusercontent.com/reearth/reearth-flow/main/engine/schema/workflow.json)
Length of output: 275
Script:
#!/bin/bash
# Description: Verify that the JSON schema includes the new sheetName property
# Expected: The schema should include a sheetName property for the FileWriter action
# Download the JSON schema and save it to a temporary file
curl -s https://raw.githubusercontent.com/reearth/reearth-flow/main/engine/schema/workflow.json -o workflow.json
# Check if the sheetName property exists in the FileWriter definition
jq '.definitions.FileWriter.properties.with.properties | has("sheetName")' workflow.json
# Clean up the temporary file
rm workflow.json
Length of output: 230
|
|
||
| pub(super) fn write_excel( | ||
| output: &Uri, | ||
| params: &ExcelWriterParam, |
There was a problem hiding this comment.
💡 Codebase verification
Incomplete update of write_excel function calls
Found a call to write_excel in writer.rs that is missing the required params argument:
engine/runtime/action-sink/src/file/writer.rs: write_excel(&output, excel_property, &self.buffer, storage_resolver)
Please update all calls to include the params argument to align with the updated function signature and prevent compile-time errors.
🔗 Analysis chain
Function signature of write_excel updated correctly
The write_excel function now includes the params parameter, aligning with the added functionality for specifying the sheet name.
Please ensure that all calls to write_excel have been updated to include the new params argument to prevent any compile-time errors.
You can run the following script to verify:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all calls to `write_excel` have been updated.
# Search for calls to `write_excel` that may not have been updated
rg --type rust 'write_excel\(' -A 2
Length of output: 499
| #[derive(Serialize, Deserialize, Debug, Clone, JsonSchema)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct ExcelWriterParam { | ||
| pub(super) sheet_name: Option<Expr>, |
There was a problem hiding this comment.
Important
The type Expr is assumed to be the target of execution of eval engine(https://github.com/reearth/reearth-flow/blob/main/engine/runtime/eval-expr/src/engine.rs). If it is just a string, then String is fine.
There was a problem hiding this comment.
I appreciate your comment😊
Fixed with 8a4ded2 👍
|
@soma00333 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
engine/runtime/action-sink/src/file/excel.rs (3)
16-20: LGTM: Well-definedExcelWriterParamstruct.The
ExcelWriterParamstruct is well-designed with appropriate derive macros and an optionalsheet_namefield. ThecamelCaserenaming is consistent with JSON conventions.Consider adding a doc comment to explain the purpose of this struct and its field:
/// Parameters for Excel writing operations. #[derive(Serialize, Deserialize, Debug, Clone, JsonSchema)] #[serde(rename_all = "camelCase")] pub struct ExcelWriterParam { /// The name of the sheet. If not provided, defaults to "Sheet1". pub(super) sheet_name: Option<String>, }
24-36: LGTM: Proper implementation of customizable sheet names.The changes to the
write_excelfunction correctly implement the new feature of customizable sheet names. The use ofparams.sheet_name.clone().unwrap_or_else(|| "Sheet1".to_string())is a good approach to handle theOption<String>.Consider using
as_deref()to avoid cloning theString:let sheet_name = params.sheet_name.as_deref().unwrap_or("Sheet1"); worksheet .set_name(sheet_name) .map_err(crate::errors::SinkError::file_writer)?;This approach is more efficient as it avoids allocating a new
Stringwhen the default value is used.
Line range hint
1-438: Overall assessment: Well-implemented feature addition.The changes to add a customizable sheet name feature are well-implemented and consistent with the existing codebase. The new
ExcelWriterParamstruct and the modifications to thewrite_excelfunction are appropriate and don't introduce any significant issues.As the
ExcelWriterParamstruct is designed to be extensible, consider the following for future development:
- If more parameters are added in the future, you might want to implement a builder pattern for
ExcelWriterParamto make it easier to construct with optional parameters.- Consider adding a
Defaultimplementation forExcelWriterParamto provide sensible defaults for all fields.These suggestions are not necessary for the current implementation but could be helpful as the feature set grows.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
engine/docs/mdbook/src/action.mdis excluded by!**/*.md
📒 Files selected for processing (2)
- engine/runtime/action-sink/src/file/excel.rs (1 hunks)
- engine/schema/actions.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- engine/schema/actions.json
🧰 Additional context used
🔇 Additional comments (1)
engine/runtime/action-sink/src/file/excel.rs (1)
13-14: LGTM: New imports for serialization and schema generation.The added imports for
JsonSchema,Serialize, andDeserializeare appropriate for the newExcelWriterParamstruct, enabling JSON schema generation and serialization/deserialization.
soma00333
force-pushed
the
feat/engine/addparameter-to-excelwrite
branch
from
917cd44 to
8a4ded2
Compare
Overview
Add
sheet_nameparameter to excel_write()Related issue: #567
What I've done
Fix example_excel.yml
Change to pass
sheetNameparameter.Add parameter to excel_write()
Add parameter to excel_write().
If parameter is given, set sheet name, if not, use
Sheet1.Define an
ExcelWriterParamstruct in case the number of parameters increases.What I haven't done
How I tested
Ran the following command.
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
sheetNameallows users to specify the name of the Excel sheet in configurations.Bug Fixes
Documentation
sheetNameproperty for better user guidance.