Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Better Dynamic converter #78

Merged
merged 16 commits into from
Feb 28, 2022
Merged

Better Dynamic converter #78

merged 16 commits into from
Feb 28, 2022

Conversation

w1ndy
Copy link
Contributor

@w1ndy w1ndy commented Feb 23, 2022

Pull Request Description

This PR improves the conversion of Dynamic into different types of values. Major changes includes:

  • Support the conversion of Dynamic into slices (as far as I know there is no way to do this with the latest release)
  • Support the conversion of Dynamic into map[string]struct
  • Reuse code for converting Dynamic into pointer values
  • Add tests

Future Release Comment

[Add description of your change, to include in the next release]
[Delete any or all irrelevant sections, e.g. if your change does not warrant a release comment at all]

Fixes:

  • Support the conversion of Dynamic into slices

@codecov-commenter
Copy link

Codecov Report

Merging #78 (502f43f) into master (ca7cd57) will increase coverage by 0.83%.
The diff coverage is 65.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
+ Coverage   47.20%   48.04%   +0.83%     
==========================================
  Files          39       39              
  Lines        4226     4196      -30     
==========================================
+ Hits         1995     2016      +21     
+ Misses       2086     2032      -54     
- Partials      145      148       +3     
Impacted Files Coverage Δ
kusto/data/value/dynamic.go 61.42% <65.21%> (+42.42%) ⬆️
kusto/test/etoe/etoe_env.go 42.00% <0.00%> (-6.00%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca7cd57...502f43f. Read the comment docs.

@AsafMah
Copy link
Contributor

AsafMah commented Feb 28, 2022

Great job!

I hope you won't mind that I did some work on the tests - modernized them and added negative tests.

@AsafMah AsafMah self-requested a review February 28, 2022 07:36
@github-actions
Copy link

github-actions bot commented Feb 28, 2022

Unit Test Results

    1 files  ±  0    13 suites  ±0   9s ⏱️ - 11m 42s
125 tests +15  112 ✔️ +7  13 💤 +13  0  - 5 

Results for commit 98c6e81. ± Comparison against base commit 75e351f.

This pull request removes 27 and adds 42 tests. Note that renamed tests count towards both.
etoe ‑ TestFileIngestion/Ingest_from_blob_with_bad_existing_mapping
etoe ‑ TestFileIngestion/Ingest_from_blob_with_existing_mapping
etoe ‑ TestFileIngestion/Ingest_from_blob_with_inline_mapping
etoe ‑ TestFileIngestion/Ingest_from_blob_with_streaming_ingestion_should_fail
etoe ‑ TestFileIngestion/Ingest_from_local_with_existing_mapping_streaming
etoe ‑ TestFileIngestion/Ingestion_from_local_file_queued
etoe ‑ TestFileIngestion/Ingestion_from_local_file_streaming
etoe ‑ TestFileIngestion/Ingestion_from_local_file_test_2_queued
etoe ‑ TestFileIngestion/Ingestion_from_local_file_test_2_streaming
etoe ‑ TestMultipleClusters/Ingestion_from_local_file_streaming
…
table ‑ TestFieldsConvert/non-valid_Bool
table ‑ TestFieldsConvert/non-valid_DateTime
table ‑ TestFieldsConvert/non-valid_Dynamic
table ‑ TestFieldsConvert/non-valid_Dynamic_list
table ‑ TestFieldsConvert/non-valid_GUID
table ‑ TestFieldsConvert/non-valid_Int
table ‑ TestFieldsConvert/non-valid_Long
table ‑ TestFieldsConvert/non-valid_String
table ‑ TestFieldsConvert/non-valid_Timespan
table ‑ TestFieldsConvert/non-valid_real
…

♻️ This comment has been updated with latest results.

@AsafMah AsafMah merged commit e6123c7 into Azure:master Feb 28, 2022
@w1ndy
Copy link
Contributor Author

w1ndy commented Mar 1, 2022

@AsafMah Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants