-
Notifications
You must be signed in to change notification settings - Fork 5
Add support for CSV/TXT text-based data files #207
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
base: develop
Are you sure you want to change the base?
Conversation
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.
I had an initial look at the proposed code and it looks good 👍 Of course need to test and dive into details to approve, but before I do that, wanted to raise a few conceptual questions to agree upon with @adecler / @alelom first:
- should CSV be supported by Excel or File toolkit? IMO file is better, as CSV is a plain, platform agnostic format
- shouldn't we split the config to
CsvPushConfigandCsvPullConfig? Some properties seem to be irrelevant on pull - I would rather avoid enforcing number formatting on
PushviaCsvConfig.Digit- if one needs to do it, prepare your data accordingly - similar with
PropertyNameand other properties - actually I would make sure it is unified with Excel_Toolkit so that interop with both tabular formats behaves the same
Immediate thoughts @linhnam-nguyen @adecler @alelom?
|
@pawelbaran Thanks a lot for taking the time to review the code.
|
|
Apologies @linhnam-nguyen for the late response, it took me a bit of time to regroup. But I think I got it! 😉 What I was concerned about was the fact that CSV interop has zero chance to be bidirectional in case of objects with nested structure, i.e. we can push chosen properties to .csv, but will never reconstruct the object back from that table on So what I think could work better in this case is supporting pushing or pulling only Table (or potentially TableRow), while all the property mapping would be contained in public static Table ToTable(this Pipe pipe)
{
// Convert the pipe to table here
}
public static Pipe FromTable(this Table table)
{
// Convert the table back to pipe here
}The above methods would not be a part of the File_Toolkit, but would sit in the project that actually leverages the interop. Naturally, Would that work for you @linhnam-nguyen, assuming you'd want to use this mechanism in Excel? When it comes to .txt interop, I would keep it simple: enable writing/reading strings to and from files, if that works for you. |
|
Hi Pawel, As you suspected, I experimented with implementing literal string read/write operations to and from a text file. However, there’s a caveat with this approach: since the table contains BHoM objects, Excel_UI recognises them as objects before any post-processing occurs, which means their string representations can’t be directly used for conversion. This, in turn, requires defining a specific output format for the BHoM objects. To make this work more seamlessly, some adjustments would be needed on the Excel_UI side — mainly to retrieve the object’s ID and type string from Excel’s cache prior to synchronisation. Unfortunately, this behaviour isn’t currently achievable within the scope of this repository. Regarding the ToTable and FromTable methods you mentioned, I believe these should be implemented directly within the UI layer, depending on how each client internalises the object structure. What are your thoughts on this approach? |
Issues addressed by this PR
Closes #206
Intended Functionality
.csvand.txtfile types.ReadFromCsvFileandWriteToCsvFilemethods with configurable options (e.g., delimiter, include headers, encoding).CsvConfigobject to control formatting and parsing.File_Enginepatterns.Expected Behaviour
Desired Inputs/Outputs
string[,],IEnumerable<IObject>).CsvConfig (available options)
\t(tab). Common alternatives:,or;.ToString()or a placeholder. If false, such objects are skipped. Default:false.true.StringType?). If null, default formatting is applied based on data type.1(true) or0(false). Default:false(booleans are text).".".null(no rounding).2023-10-05T14:48:00Z10/05/202305/10/2023Default:
EU.Test files
Test.xlsx
Changelog
Additional comments
Nested enums cannot currently be parsed directly from
Excel_UIstrings; only numeric values are supported. Additional work in the relevant Repos would be required to enable robust enum (and nested enum) parsing from strings.