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

feat(sdk): add print function to create console output without a trialing newline #5379

Closed
wants to merge 4 commits into from

Conversation

mortax
Copy link
Collaborator

@mortax mortax commented Jan 1, 2024

This PR implements the real solution to the issue raised in discussion #1772:

  • Add print() to generate console output without a trailing newline

Note that I have not included println() since it would be the same as log().

By submitting this pull request, I confirm that my contribution is made under the terms of the Wing Cloud Contribution License.

@mortax mortax requested a review from a team as a code owner January 1, 2024 01:38
@monadabot
Copy link
Contributor

Thanks for opening this pull request! 🎉
Please consult the contributing guidelines for details on how to contribute to this project.
If you need any assistence, don't hesitate to ping the relevant owner over Slack.

Topic Owner
Wing SDK and utility APIs @chriscbr
Wing Console @ainvoner, @skyrpex, @polamoros
JSON, structs, primitives and collections @hasanaburayyan
Platforms and plugins @hasanaburayyan
Frontend resources (website, react, etc) @tsuf239
Language design @eladb
VSCode extension and language server @markmcculloh
Compiler architecture, inflights, lifting @yoav-steinberg
Wing Testing Framework @tsuf239
Wing CLI @markmcculloh
Build system, dev environment, releases @markmcculloh
Library Ecosystem @chriscbr
Documentation @hasanaburayyan
SDK test suite @tsuf239
Examples @skorfmann
Wing Playground @eladcon

@monadabot
Copy link
Contributor

monadabot commented Jan 1, 2024

Console preview environment is available at https://wing-console-pr-5379.fly.dev 🚀

Last Updated (UTC) 2024-01-01 07:28

@monadabot
Copy link
Contributor

monadabot commented Jan 1, 2024

Benchmarks

Comparison to Baseline 🟥⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜
Benchmark Before After Change
version 75ms±0.6 78ms±0.9 +3ms (+3.88%)🟥
jsii_big.test.w -t sim 3519ms±23.16 3531ms±18.61 +12ms (+0.34%)⬜
jsii_big.test.w -t tf-aws 3640ms±14.1 3644ms±16.02 +4ms (+0.11%)⬜
functions_1.test.w -t sim 653ms±5.13 656ms±6.32 +3ms (+0.47%)⬜
functions_1.test.w -t tf-aws 1827ms±50.25 1794ms±34.82 -33ms (-1.82%)⬜
hello_world.test.w -t sim 656ms±6.72 653ms±5.69 -4ms (-0.58%)⬜
hello_world.test.w -t tf-aws 5140ms±44.32 5132ms±38.53 -8ms (-0.16%)⬜
functions_10.test.w -t sim 722ms±4.53 724ms±5.14 +2ms (+0.21%)⬜
functions_10.test.w -t tf-aws 3790ms±32.69 3846ms±14.36 +56ms (+1.48%)⬜
jsii_small.test.w -t sim 607ms±7.26 613ms±6.45 +6ms (+0.95%)⬜
jsii_small.test.w -t tf-aws 729ms±5.21 738ms±2.54 +9ms (+1.27%)⬜
empty.test.w -t sim 613ms±4.2 615ms±5.42 +2ms (+0.33%)⬜
empty.test.w -t tf-aws 737ms±6.39 732ms±5.3 -5ms (-0.7%)⬜

⬜ Within 1.5 standard deviations
🟩 Faster, Above 1.5 standard deviations
🟥 Slower, Above 1.5 standard deviations

Benchmarks may vary outside of normal expectations, especially when running in GitHub Actions CI.

Results
name mean min max moe sd
version 78ms 76ms 80ms 1ms 1ms
jsii_big.test.w -t sim 3531ms 3491ms 3579ms 19ms 26ms
jsii_big.test.w -t tf-aws 3644ms 3601ms 3677ms 16ms 22ms
functions_1.test.w -t sim 656ms 638ms 668ms 6ms 9ms
functions_1.test.w -t tf-aws 1794ms 1746ms 1893ms 35ms 49ms
hello_world.test.w -t sim 653ms 639ms 664ms 6ms 8ms
hello_world.test.w -t tf-aws 5132ms 5010ms 5194ms 39ms 54ms
functions_10.test.w -t sim 724ms 714ms 737ms 5ms 7ms
functions_10.test.w -t tf-aws 3846ms 3824ms 3888ms 14ms 20ms
jsii_small.test.w -t sim 613ms 600ms 626ms 6ms 9ms
jsii_small.test.w -t tf-aws 738ms 731ms 745ms 3ms 4ms
empty.test.w -t sim 615ms 603ms 626ms 5ms 8ms
empty.test.w -t tf-aws 732ms 724ms 745ms 5ms 7ms
Last Updated (UTC) 2024-01-01 07:36

@monadabot monadabot added the ⚠️ pr/review-mutation PR has been mutated and will not auto-merge. Clear this label if the changes look good! label Jan 1, 2024
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

I am not convinced that this use case entitles a whole new built in function. We are trying to minimize those as much as possible.

Maybe something like fs.stdout.write("text") work?

bring cloud;


log("preflight log");
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this to test under sdk_tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure. interestingly, this was already in valid but you're right, sdk_tests makes much more sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe something like fs.stdout.write("text") work?

yes, that would certainly work (i have been doing the equivalent). thought i completely understand the goal to minimize, i think any language that supports basic output (as Wing does with log()) should do the same without a newline (just MHO).

Copy link
Contributor

Choose a reason for hiding this comment

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

How about?

log("foo", newline: false);

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a bit of context: logging in wing doesn't have the same semantic meaning as printing to standard error, standard output in traditional programming, languages.

There's also a big difference between preflight and inflight.

In preflight logs are basically part of the compilation output while in inflight logs are emitted from the runtime code, executed on the cloud in various computation platforms, such as lambda containers, etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i gave it a try (spending most of today messing with it) but didn't have any real success.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mortax really appreciate the efforts!

@Chriscbr or @yoav-steinberg will be able to help.

alternatively - if you are comfortable with implementing this as a standard library function in the meantime, so that you are unblocked, then go ahead and we can discuss the built-in support in log() separately.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks @eladb. i can wait until it can be done properly. i have my own workaround for now so it's not worth expending effort for something temporary.

i'm ready to do the bulk of the work -- i just need a gentle shove in the right direction.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you're on the right track @mortax. To get the newline parameter working, I think you'll need to add a new struct type. The reason's that in Wing, named parameters are implemented by providing a struct as the last parameter in any function or constructor. So it will be as if we defined the log function like

let log = (message: str, opts: LogOptions) => { ... }

but callers can use it any of these ways:

log("foo");
// or
log("foo", newline: false);
// or
log("foo", LogOptions { newline: false });

I'd suggest maybe going about this by adding a struct through the Wing SDK, which is implemented as a TypeScript interface. Here's an example of another one:

/**
* Options for stringify() method.
*/
export interface JsonStringifyOptions {
/** Indentation spaces number */
readonly indent: number;
}

std would be a good place for this. Once the interface is added there, you can update the code where builtins are added. (It was in lib.rs but it was recently moved to type_check.rs - look for the add_builtins function).

Let me know that helps or if you run into any issues!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks. i'll try to give this a try tomorrow.

monadabot and others added 3 commits January 1, 2024 09:23
Signed-off-by: monada-bot[bot] <monabot@monada.co>
Signed-off-by: monada-bot[bot] <monabot@monada.co>
@mortax mortax marked this pull request as draft January 1, 2024 16:45
Copy link

Hi,

This PR has not seen activity in 20 days. Therefore, we are marking the PR as stale for now. It will be closed after 7 days.
If you need help with the PR, do not hesitate to reach out in the winglang community slack at winglang.slack.com.
Feel free to re-open this PR when it is still relevant and ready to be worked on again.
Thanks!

@github-actions github-actions bot added the Stale label Jan 25, 2024
@github-actions github-actions bot closed this Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ pr/review-mutation PR has been mutated and will not auto-merge. Clear this label if the changes look good! Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants