ADR for adding option for CLI output to CSV #310
Conversation
pritipath
force-pushed
the
adr-cli-output-csv
branch
from
5af93af to
7cc3448
Compare
pritipath
force-pushed
the
adr-cli-output-csv
branch
from
0aad076 to
0b4757a
Compare
I, Priti <[email protected]>, hereby add my Signed-off-by to this commit: 0b4757a Signed-off-by: Priti <[email protected]>
|
Related to #35 |
|
Discussed in #323 , week to discuss and approve if no objections |
There was a problem hiding this comment.
@pritipath This ADR does a nice job of covering the many different considerations. For a next rev, I would love for it be a bit more opinionated so that someone going to implement such a feature doesn't need to grapple with all of it.
My personal opinion:
- I am usually for not reinventing the wheel and using 3rd Party packages for CSV. But since we only need it for output, unless there is a small/modular package that does only what we need, it may be better to just write it ourself. Definitely depends on some of the answers to my in-line locale question.
- For forecasting my preference would be, having a row for each flattened
ForecastDatavalue and a column for each flattenedOptimalDataPointvalue. Specifically putting the OptimalDataPoints at the end so that it's easier to programmatically manage them since the first 10 columns will always be the same, and the remaining columns can be parsed as optimal value pairs. EG:
| RequestedAt | Location | ... | ForecastDataTime | ForecastDataRating | OptimalTime_0 | OptimalRating_0 | OptimalTime_1 | OptimalRating_1 |
|---|---|---|---|---|---|---|---|---|
| 2023-04-01T00:00:00Z | EastUS | ... | 2023-04-01T01:00:00Z | 123 | 2023-04-01T01:00:00Z | 123 | 2023-04-01T02:00:00Z | 123 |
| 2023-04-01T00:00:00Z | EastUS | ... | 2023-04-01T02:00:00Z | 123 | 2023-04-01T01:00:00Z | 123 | 2023-04-01T02:00:00Z | 123 |
| 2023-04-01T00:00:00Z | EastUS | ... | 2023-04-01T03:00:00Z | 456 | 2023-04-01T01:00:00Z | 123 | 2023-04-01T02:00:00Z | 123 |
| westeurope,2023-01-31T15:00:00.0000000+00:00,196,PT1H | ||
| westeurope,2023-01-31T16:00:00.0000000+00:00,236,PT1H | ||
| ``` | ||
| ### Limitations of CSV format |
There was a problem hiding this comment.
Different locales may also use commas instead of decimals. It would be good to understand how the locales in the SDK would work (or not) with CSV implementations.
Do we need to be able to specify a custom separator? In config? As a command line option? Do CSV libraries default to tabs or pipes in other locales?
|
Reviewed during #334 , looking to approve. |
Pull Request
Summary
ADR for providing an option to display CLI output in CSV format
Checklist
Are there API Changes?
If yes, what are the expected API Changes? Please link to an API-Comparison
workflow with the API Diff.
Is this a breaking change?
If yes, what workflow does this break?
Anything else?
Other comments, collaborators, etc.
This PR Closes #<issue_number>