Skip to content
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

Allow exporting specific columns #2135

Merged
merged 7 commits into from
Oct 25, 2024
Merged

Conversation

DmitryAstafyev
Copy link
Collaborator

Introduce changes, which gives user a way to export data into text file only with specific (selected) columns.

Added

  • API on rustcore
  • API on electron side and on client side
  • New dialog on client

Comment on lines +110 to +115
/// The indices of the columns to include in the export.
columns: Vec<usize>,
/// An optional string used as the record separator in session file to split log message to columns. Defaults can be applied if `None`.
spliter: Option<String>,
/// An optional string used as the field delimiter within each record in output file. Defaults can be applied if `None`.
delimiter: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think those fields could be grouped in an optional struct, since they can have values always together. (There is no path in code for a case where a splitter is provided without a delimiter and the columns is used only on the case where both splitter and delimiter are provided)

Maybe a struct like ColumnsModificationSettings that have them all which can be wrapped inside an Option as a whole would be clearer. For sure this change can be carried to all the other modified export function and structs

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I also thought about it. I've refused from object because of node-bindgen. Such an object would also go there, and it will require using of something like serde_json, which I'm trying to avoid considering possibly switching to protobuf way. Because if we will move to protobuf, I think we will also get rid of serde_json at all. Well that's why I left it for now "flat".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and in the scope of "move to protobuf" we will group arguments of all public API into messages to switch it also to proto and make more structured.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can consider the option to have the new struct on the levels where node-bindgen doesn't have access.

I think this can start before the function state.export_sesssion(...) on line 348 (if it's not a part of the bindgen), at that point we can map those three fields to one struct and use it in the rest of Rust code.
Then it would be easier to replace the three fields with the struct once "move to protobuf" is done because we would already have the struct and we just need to export it and remove the mapping in one place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This enum element (and others enums too) as itself is already a struct of options of export.

Export {
        /// The output path where the exported data will be written.
        out_path: PathBuf,
        /// The ranges of data to be exported, each defined as an inclusive range.
        ranges: Vec<std::ops::RangeInclusive<u64>>,
        /// The indices of the columns to include in the export.
        columns: Vec<usize>,
        /// An optional string used as the record separator in session file to split log message to columns. Defaults can be applied if `None`.
        spliter: Option<String>,
        /// An optional string used as the field delimiter within each record in output file. Defaults can be applied if `None`.
        delimiter: Option<String>,
    }

Introducing here an additional level of nesting looks to me excessive, even if some of the fields are used in pairs.

But I agree we need grouping on level of public API:

    pub fn export(
        &self,
        operation_id: Uuid,
        out_path: PathBuf,
        ranges: Vec<RangeInclusive<u64>>,
        columns: Vec<usize>,
        spliter: Option<String>,
        delimiter: Option<String>,
    ) -> Result<(), ComputationError> {

That's obviously not good to have so many arguments. But this is a common problem and it's out of the scope of current changes. In general, we should introduce a common API interface to come to some uniformity for all public methods. Let's say with only 2 arguments always: ident, options. And also it's related again to protobuf integration because it's much closer to this subject.

@DmitryAstafyev DmitryAstafyev merged commit a7c83a5 into esrlabs:master Oct 25, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants