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

fix: Cannot import CSV with LOCAL_TIME #1434

Merged
merged 3 commits into from
Aug 1, 2023

Conversation

mofojed
Copy link
Member

@mofojed mofojed commented Aug 1, 2023

  • Fix adding error console history items
    • They weren't appearing as anything because the command was '' instead of undefined
  • Map LOCAL_TIME to STRING on CSV import
    • LOCAL_TIME isn't supported on the server in DHC, and isn't really "safe" anyways since we should have other info in there as well (e.g. date or time zone)
    • When exporting, should be exporting the 'unformatted' value, which gives you the full timestamp
    • By mapping to String, user can then do an .update_view to map it to a timestamp if they wish
  • Fixes Cannot import CSV, get a Can't parse LOCAL_TIME exception in the logs, no error shown #1432

mofojed added 2 commits August 1, 2023 16:58
- We were adding an empty string as the command, which prevented it from appearing as an error in the console history
- LOCAL_TIME is not supported on the DHC server currently
- Rather than erroring out, just have it map to a String. User can then map to an Instant or something with an `update_view`.
@mofojed mofojed requested a review from mattrunyon August 1, 2023 21:19
@mofojed mofojed self-assigned this Aug 1, 2023
@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #1434 (0154fc9) into main (e3db6bf) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #1434      +/-   ##
==========================================
- Coverage   45.71%   45.71%   -0.01%     
==========================================
  Files         511      511              
  Lines       35070    35073       +3     
  Branches     8769     8772       +3     
==========================================
+ Hits        16033    16034       +1     
- Misses      18986    18988       +2     
  Partials       51       51              
Flag Coverage Δ
unit 45.71% <50.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
packages/console/src/Console.tsx 18.67% <ø> (ø)
packages/console/src/csv/CsvTypeParser.ts 45.51% <0.00%> (-0.64%) ⬇️
...console/src/console-history/ConsoleHistoryItem.tsx 52.72% <100.00%> (+0.87%) ⬆️

@mattrunyon
Copy link
Collaborator

mattrunyon commented Aug 1, 2023

This seems to have broken my console and it doesn't display any of my previously run code (error or not). No browser errors logged

image

@mofojed mofojed changed the title fix: Cannot import CSV with timestamp fix: Cannot import CSV with LOCAL_TIME Aug 1, 2023
@mofojed mofojed merged commit caa6bc8 into deephaven:main Aug 1, 2023
@mofojed mofojed deleted the 1432-csv-import-error branch August 1, 2023 22:54
@github-actions github-actions bot locked and limited conversation to collaborators Aug 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot import CSV, get a Can't parse LOCAL_TIME exception in the logs, no error shown
2 participants