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

Declare parts of std in KCL rather than Rust #5147

Merged
merged 1 commit into from
Feb 20, 2025
Merged

Declare parts of std in KCL rather than Rust #5147

merged 1 commit into from
Feb 20, 2025

Conversation

nrc
Copy link
Contributor

@nrc nrc commented Jan 24, 2025

This PR lets us declare consts and functions (which can be written in either KCL (no example in this PR) or Rust) in std using KCL (which lets us have consts in std and thus in code-completions and docs), it moves the in-memory constants to std and introduces constants to replace the (now-deprecated) functions for maths constants (the latter are breaking changes, but I've left the old functions for now). I've moved cos to KCL as a test case (not a breaking change). This PR shouldn't break any code.

Outstanding work:

  • support KW calls to KCL std functions
  • doc comments on args (or extract them from the function comment)
  • open std in editor for jump to def, etc.
  • docs on types in KCL std
  • migrate other functions and types to KCL
  • let std functions refer to their environment (currently can't be done but not an issue since they're all in Rust anyway)

Copy link

qa-wolf bot commented Jan 24, 2025

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

Copy link

vercel bot commented Jan 24, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview Feb 20, 2025 5:10am

@nrc nrc requested a review from adamchalmers January 24, 2025 04:18
@nrc nrc force-pushed the nrc-std-2 branch 2 times, most recently from 3e7c648 to f8a47c6 Compare January 24, 2025 04:27
@nrc nrc force-pushed the nrc-std-2 branch 2 times, most recently from 381f6ce to 18fdd1c Compare January 28, 2025 04:03
@nrc nrc force-pushed the nrc-std-2 branch 2 times, most recently from 280861b to 87f6cef Compare February 11, 2025 01:01
Copy link

codecov bot commented Feb 13, 2025

Codecov Report

Attention: Patch coverage is 85.06637% with 135 lines in your changes missing coverage. Please review.

Project coverage is 86.10%. Comparing base (099c48c) to head (98efbdb).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/wasm-lib/kcl/src/docs/kcl_doc.rs 84.26% 73 Missing ⚠️
src/wasm-lib/kcl/src/parsing/ast/types/mod.rs 56.60% 23 Missing ⚠️
src/wasm-lib/kcl/src/modules.rs 36.00% 16 Missing ⚠️
src/wasm-lib/kcl/src/execution/exec_ast.rs 91.66% 11 Missing ⚠️
src/wasm-lib/kcl/src/execution/mod.rs 79.16% 5 Missing ⚠️
src/wasm-lib/kcl/src/docs/gen_std_tests.rs 97.84% 3 Missing ⚠️
src/wasm-lib/kcl/src/lsp/kcl/mod.rs 81.25% 3 Missing ⚠️
src/wasm-lib/kcl/src/std/mod.rs 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5147      +/-   ##
==========================================
+ Coverage   85.87%   86.10%   +0.22%     
==========================================
  Files          94       95       +1     
  Lines       34818    35480     +662     
==========================================
+ Hits        29901    30551     +650     
- Misses       4917     4929      +12     
Flag Coverage Δ
wasm-lib 86.10% <85.06%> (+0.22%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

}]
fn inner_cos(num: f64) -> Result<f64, KclError> {
Ok(num.cos())
Ok(args.make_user_val_from_f64_with_type(num.cos(), NumericType::count()))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be Radians not Count numeric type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so - this is the type of the result of cos, which is a ratio (if I remember high school maths properly :-)) so should be a Count

@nrc nrc merged commit 7500ef0 into main Feb 20, 2025
56 of 67 checks passed
@nrc nrc deleted the nrc-std-2 branch February 20, 2025 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants