Base32 Module Integration for KCL Standard Library #1883

gitatractivo · 2025-03-08T18:27:30Z

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

N
Y

2. What is the scope of this PR (e.g. component or file name):

kclvm/runtime/src/base32/mod.rs
kclvm/sema/src/builtin/system_module.rs
kclvm/tests/test_units/runtime/base32/test_base32.py

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

N
Y

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

Added unit tests in kclvm/tests/test_units/runtime/base32/test_base32.py that verify encoding and decoding functionality using RFC 4648 test vectors. The tests follow the same pattern as the existing Base64 tests.

zong-zhe · 2025-03-11T02:32:35Z

Hi @gitatractivo 😄

Good work! 👍👍👍 Now you need to tidy up your commits, squash them into one commit, and use git commit -s to make DCO checks pass.

zong-zhe · 2025-03-11T02:33:55Z

LGTM, and cc @He1pa

zong-zhe · 2025-03-11T03:54:51Z

Hi @gitatractivo 😄

A test case failed, cc@He1pa, could you provide some more details to help him fix the test 😄

He1pa · 2025-03-12T03:59:48Z

update the ut

gitatractivo · 2025-03-12T04:30:53Z

Hi,
I’ve squashed and committed the changes—please let me know if everything looks good!

He1pa · 2025-03-12T08:53:44Z

run failed ut locally and update snapshot by cargo insta review

gitatractivo · 2025-03-16T07:36:46Z

Hi I tried to replicate this on windows.
I ran cargo test --workspace -r -- --nocapture and got same error

The output of cargo insta review was:
done: no snapshots to review

He1pa · 2025-03-17T09:58:07Z

Hi I tried to replicate this on windows. I ran cargo test --workspace -r -- --nocapture and got same error

The output of cargo insta review was: done: no snapshots to review

Did you generate a .snap.new file?

gitatractivo · 2025-03-18T12:49:19Z

No I did not generate a new file.

gitatractivo · 2025-03-19T05:47:28Z

Hi @He1pa, I need help with passing these tests. I am getting this error while doing make test.
I even cloned repo again and ran make test again but still getting same error.

He1pa · 2025-03-19T05:53:02Z

Hi @He1pa, I need help with passing these tests. I am getting this error while doing make test. I even cloned repo again and ran make test again but still getting same error.

These ut related to kpm, it need to download some pkgs. you can ignore these changes and just update import_builtin_package

gitatractivo · 2025-03-19T05:58:57Z

How will these failing test cases pass? Is there something else I need to do to close this PR?

He1pa · 2025-03-19T06:06:47Z

How will these failing test cases pass? Is there something else I need to do to close this PR?

you can check failed ci. update test snapshot in kclvm/loader/*

gitatractivo · 2025-03-19T14:10:31Z

I fixed this test but there are 7 other failing 6 of them are related to file import error and 1 related to module path incorrect error.

He1pa · 2025-03-21T06:59:41Z

Author
I saw that these tests failed in CI. This is a normal change after the builtin changes. Just update the snapshot here. You can run the tests in the loader directory independently, ignoring the tests in lsp.
failures:
tests::assign_stmt_0
tests::assign_stmt_1
tests::assign_stmt_2
tests::builtin_call_0
tests::builtin_call_1
tests::builtin_call_2
tests::import_stmt_0
tests::import_stmt_1

gitatractivo · 2025-03-21T09:02:38Z

I updated the loader snapshots with cargo insta. All the tests in loader directory are passing but when doing make test in root directory getting these tests failed

failures:
    tests::complete_import_external_file_e2e_test
    tests::konfig_completion_test_main
    tests::konfig_goto_def_test_base
    tests::konfig_goto_def_test_main
    tests::konfig_hover_test_main
    tests::pkg_mod_test
    tests::test_lsp_with_kcl_mod_in_order

He1pa · 2025-03-21T09:06:37Z

If there are no changes, ci will pass these tests. But you need fmt code @gitatractivo

gitatractivo · 2025-03-24T07:18:35Z

Hi, @He1pa sorry to trouble you again and again. I am not able to pass this last test pkg_mod_test. I tried to debug I am getting module not found errors in LSP/src/test.rs for importing src/test_data/workspace/pkg_mod_test/test/main.k

kclvm/tools/src/LSP/src/test_data/workspace/pkg_mod_test/test/kcl.mod

He1pa · 2025-03-24T07:27:59Z

Hi, @He1pa sorry to trouble you again and again. I am not able to pass this last test pkg_mod_test. I tried to debug I am getting module not found errors in LSP/src/test.rs for importing src/test_data/workspace/pkg_mod_test/test/main.k

it seems you edit the mod file about this test, maybe we should restored this?

gitatractivo · 2025-03-24T07:45:18Z

Yes I restored the path.

gitatractivo · 2025-03-24T08:32:06Z

A new test failed now

failures:
    resolver::tests::test_system_package

kclvm/sema/src/resolver/tests.rs

coveralls · 2025-03-24T09:25:06Z

Pull Request Test Coverage Report for Build 14033002206

Details

24 of 56 (42.86%) changed or added relevant lines in 4 files are covered.
4 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.001%) to 70.35%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
kclvm/runtime/src/base32/mod.rs	0	32	0.0%

Files with Coverage Reduction	New Missed Lines	%
kclvm/config/src/settings.rs	4	72.21%

Totals
Change from base Build 13890938123:	-0.001%
Covered Lines:	52665
Relevant Lines:	74861

💛 - Coveralls

gitatractivo · 2025-03-24T09:51:20Z

failures:
    api/src/service/service_impl.rs - service::service_impl::KclvmServiceImpl::load_package (line 205)

this test fails

I found that values for left and right are different for file /kclvm/api/src/service/service_impl.rs line 226-228

 /// assert_eq!(result.node_symbol_map.len(), 187);  -> left 189
 /// assert_eq!(result.symbol_node_map.len(), 187);   -> left 189
 /// assert_eq!(result.fully_qualified_name_map.len(), 197);  -> left 200

But test passed for macos? I did not get this.

He1pa · 2025-03-24T10:25:23Z

failures:
    api/src/service/service_impl.rs - service::service_impl::KclvmServiceImpl::load_package (line 205)
this test fails

I found that values for left and right are different for file /kclvm/api/src/service/service_impl.rs line 226-228
 /// assert_eq!(result.node_symbol_map.len(), 187);  -> left 189
 /// assert_eq!(result.symbol_node_map.len(), 187);   -> left 189
 /// assert_eq!(result.fully_qualified_name_map.len(), 197);  -> left 200
  
But test passed for macos? I did not get this.

I think it should be 189？ change it to 189 and test again?

- Added BASE32 constant to STANDARD_SYSTEM_MODULES list - Implemented base32 encoding and decoding functions - Updated test files to use the correct Base32 encoding/decoding - Fixed imports in test files - Updated completion test to include base32 module - Ensured all tests pass with the new module Fixes kcl-lang#1860 Signed-off-by: Gitanshu Talwar <[email protected]> Reverted kcl.mod file Signed-off-by: Gitanshu Talwar <[email protected]> - Added base32 in test_data/system_package.k Signed-off-by: Gitanshu Talwar <[email protected]> - Changing assertion

gitatractivo · 2025-03-24T10:34:28Z

failures:
    api/src/service/service_impl.rs - service::service_impl::KclvmServiceImpl::load_package (line 205)
this test fails
I found that values for left and right are different for file /kclvm/api/src/service/service_impl.rs line 226-228
 /// assert_eq!(result.node_symbol_map.len(), 187);  -> left 189
 /// assert_eq!(result.symbol_node_map.len(), 187);   -> left 189
 /// assert_eq!(result.fully_qualified_name_map.len(), 197);  -> left 200
  
But test passed for macos? I did not get this.
I think it should be 189？ change it to 189 and test again?

Changed to 189 189 200

He1pa

LGTM

He1pa · 2025-03-24T12:20:22Z

cc @zong-zhe

zong-zhe

LGTM

gitatractivo force-pushed the base32-stndlib branch from bc8cb35 to 6a6198e Compare March 12, 2025 04:29

gitatractivo force-pushed the base32-stndlib branch from 6a6198e to 183ebf0 Compare March 18, 2025 13:45

gitatractivo force-pushed the base32-stndlib branch from 183ebf0 to 807555f Compare March 21, 2025 09:00

gitatractivo force-pushed the base32-stndlib branch 3 times, most recently from ee6064e to 2b3f7bd Compare March 21, 2025 09:46

He1pa reviewed Mar 24, 2025

View reviewed changes

kclvm/tools/src/LSP/src/test_data/workspace/pkg_mod_test/test/kcl.mod Outdated Show resolved Hide resolved

gitatractivo force-pushed the base32-stndlib branch from 2b3f7bd to 9510bf2 Compare March 24, 2025 07:33

He1pa reviewed Mar 24, 2025

View reviewed changes

kclvm/sema/src/resolver/tests.rs Show resolved Hide resolved

gitatractivo force-pushed the base32-stndlib branch from 9510bf2 to 98c1682 Compare March 24, 2025 08:38

gitatractivo force-pushed the base32-stndlib branch from 98c1682 to 666623c Compare March 24, 2025 10:31

He1pa approved these changes Mar 24, 2025

View reviewed changes

zong-zhe approved these changes Mar 24, 2025

View reviewed changes

zong-zhe merged commit 309b593 into kcl-lang:main Mar 24, 2025
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Base32 Module Integration for KCL Standard Library #1883

Base32 Module Integration for KCL Standard Library #1883

gitatractivo commented Mar 8, 2025 •

edited

Loading

zong-zhe commented Mar 11, 2025

zong-zhe commented Mar 11, 2025

zong-zhe commented Mar 11, 2025

He1pa commented Mar 12, 2025

gitatractivo commented Mar 12, 2025 •

edited

Loading

He1pa commented Mar 12, 2025

gitatractivo commented Mar 16, 2025 •

edited

Loading

He1pa commented Mar 17, 2025

gitatractivo commented Mar 18, 2025 •

edited

Loading

gitatractivo commented Mar 19, 2025 •

edited

Loading

He1pa commented Mar 19, 2025 •

edited

Loading

gitatractivo commented Mar 19, 2025

He1pa commented Mar 19, 2025 •

edited

Loading

gitatractivo commented Mar 19, 2025

He1pa commented Mar 21, 2025

gitatractivo commented Mar 21, 2025 •

edited

Loading

He1pa commented Mar 21, 2025

gitatractivo commented Mar 24, 2025

He1pa commented Mar 24, 2025

gitatractivo commented Mar 24, 2025

gitatractivo commented Mar 24, 2025

coveralls commented Mar 24, 2025 •

edited

Loading

gitatractivo commented Mar 24, 2025 •

edited

Loading

He1pa commented Mar 24, 2025

gitatractivo commented Mar 24, 2025

He1pa left a comment

He1pa commented Mar 24, 2025

zong-zhe left a comment

Base32 Module Integration for KCL Standard Library #1883

Base32 Module Integration for KCL Standard Library #1883

Conversation

gitatractivo commented Mar 8, 2025 • edited Loading

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

2. What is the scope of this PR (e.g. component or file name):

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

zong-zhe commented Mar 11, 2025

zong-zhe commented Mar 11, 2025

zong-zhe commented Mar 11, 2025

He1pa commented Mar 12, 2025

gitatractivo commented Mar 12, 2025 • edited Loading

He1pa commented Mar 12, 2025

gitatractivo commented Mar 16, 2025 • edited Loading

He1pa commented Mar 17, 2025

gitatractivo commented Mar 18, 2025 • edited Loading

gitatractivo commented Mar 19, 2025 • edited Loading

He1pa commented Mar 19, 2025 • edited Loading

gitatractivo commented Mar 19, 2025

He1pa commented Mar 19, 2025 • edited Loading

gitatractivo commented Mar 19, 2025

He1pa commented Mar 21, 2025

gitatractivo commented Mar 21, 2025 • edited Loading

He1pa commented Mar 21, 2025

gitatractivo commented Mar 24, 2025

He1pa commented Mar 24, 2025

gitatractivo commented Mar 24, 2025

gitatractivo commented Mar 24, 2025

coveralls commented Mar 24, 2025 • edited Loading

Pull Request Test Coverage Report for Build 14033002206

Details

💛 - Coveralls

gitatractivo commented Mar 24, 2025 • edited Loading

He1pa commented Mar 24, 2025

gitatractivo commented Mar 24, 2025

He1pa left a comment

Choose a reason for hiding this comment

He1pa commented Mar 24, 2025

zong-zhe left a comment

Choose a reason for hiding this comment

gitatractivo commented Mar 8, 2025 •

edited

Loading

gitatractivo commented Mar 12, 2025 •

edited

Loading

gitatractivo commented Mar 16, 2025 •

edited

Loading

gitatractivo commented Mar 18, 2025 •

edited

Loading

gitatractivo commented Mar 19, 2025 •

edited

Loading

He1pa commented Mar 19, 2025 •

edited

Loading

He1pa commented Mar 19, 2025 •

edited

Loading

gitatractivo commented Mar 21, 2025 •

edited

Loading

coveralls commented Mar 24, 2025 •

edited

Loading

gitatractivo commented Mar 24, 2025 •

edited

Loading