-
Notifications
You must be signed in to change notification settings - Fork 56
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
better track the root source code #5558
Conversation
Signed-off-by: Jess Frazelle <[email protected]>
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I feel like now that we have the source for the main module we should be able to tidy up the module infos etc. to treat the main module more like the others, but I'm not sure how to actually do that.
src/wasm-lib/kcl/src/lib.rs
Outdated
@@ -171,6 +185,7 @@ impl Program { | |||
pub fn change_meta_settings(&mut self, settings: crate::MetaSettings) -> Result<Self, KclError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a weird method because it looks like it mutates the self
passed in and returns a clone of itself? I'm a bit averse to cloning the program text (and the AST?) here, so could it take self
by value or just mutate self
rather than cloning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah okay wasnt part of this pr but i can fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
I think we will always need helpers that take in raw code but like having a helper that takes a path to a dir or file and if dir ties to find a main.kcl (since we standardize assemblies on that) would be fucking nice and then i think we can do what you say. |
Signed-off-by: Jess Frazelle <[email protected]>
Signed-off-by: Jess Frazelle <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5558 +/- ##
==========================================
- Coverage 86.43% 86.42% -0.02%
==========================================
Files 97 97
Lines 37194 37224 +30
==========================================
+ Hits 32150 32169 +19
- Misses 5044 5055 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@nrc let me know what you think of this change, basically before we could store all the source code files for modules but getting the root was hard since we parse and read the file out of step