Skip to content

Conversation

jtran
Copy link
Contributor

@jtran jtran commented Sep 3, 2025

Extracted from #8103.

Before, types of function parameters did not get serialized. So using them in the app had no effect even though they should coerce arguments to the parameter type, including converting units.

I see this as a minor breaking change. I expect it to impact few users since few KCL programs use parameter types to coerce.

Copy link

vercel bot commented Sep 3, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
modeling-app Ready Ready Preview Comment Sep 3, 2025 4:22pm

@jtran jtran changed the title Don't erase param types during serlialisation Fix KCL function parameter unit conversion to work in ZDS Sep 3, 2025
Copy link

codspeed-hq bot commented Sep 3, 2025

CodSpeed Instrumentation Performance Report

Merging #8205 will not alter performance

Comparing jtran/fix-param-coercion (14abfd0) with main (4503aa4)1

Summary

✅ 89 untouched benchmarks

Footnotes

  1. No successful run was found on main (4f4fa99) during the generation of this report, so 4503aa4 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@jtran jtran marked this pull request as ready for review September 3, 2025 16:33
@jtran jtran requested a review from a team as a code owner September 3, 2025 16:33
.unwrap();
let mut exec_state = ExecState::new(&ctx);
ctx.run(&program, &mut exec_state).await.map_err(|e| e.error).unwrap();
println!("{:#?}", exec_state.errors());
Copy link
Contributor

Choose a reason for hiding this comment

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

should probably be eprintln and should be wrapped in if !exec_state.errors().is_empty()

Copy link
Contributor

Choose a reason for hiding this comment

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

The output from main.rs is very debug-y and I found it useful to have the [] as an indicator of no errors.

To be pedantic, it's reporting an error with the user's code not reporting an error with the program, so I think it should be using print rather than eprint

Copy link
Contributor

@adamchalmers adamchalmers left a comment

Choose a reason for hiding this comment

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

Approved with one nit

@nrc
Copy link
Contributor

nrc commented Sep 3, 2025

I see this as a minor breaking change. I expect it to impact few users since few KCL programs use parameter types to coerce.

It's also a bug fix, so fine to land according to our breaking change policy

@nrc nrc merged commit f7c7823 into main Sep 3, 2025
88 of 89 checks passed
@nrc nrc deleted the jtran/fix-param-coercion branch September 3, 2025 20:32
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.

3 participants