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(cli): adding pre-release + analytics disclaimer on first run of cli #3058

Closed
wants to merge 4 commits into from

Conversation

garysassano
Copy link
Collaborator

Closes #2974

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

@garysassano garysassano requested a review from a team as a code owner June 26, 2023 02:41
@garysassano garysassano changed the title feat(cli): adding pre-release +analytics disclaimer on first run feat(cli): adding pre-release + analytics disclaimer on first run Jun 26, 2023
@garysassano garysassano changed the title feat(cli): adding pre-release + analytics disclaimer on first run feat(cli): adding pre-release + analytics disclaimer on first run of cli Jun 26, 2023
"https://winglang.io/docs/analytics"
)}\n` +
`\n` +
`${chalk.redBright("(This message will self-destruct after the first run)")}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please set those as an array of strings, and join them with "\n"?
(it will make it easier to read)
another option is to write the whole paragraph inside of ` ` as it will allow you to write the entire section without extra characters

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tsuf239 Actually I had already considered everything you said. I looked at all the possible ways to achieve multiline string and several people deemed the string array to be "overkill", even tho I actually liked that solution the most. Then I used the template literal but the result ended up being pretty messy (had to create a regex to remove trailing spaces since all lines were indented and account also for the blank lines) so I rewrote everything using strings chained with plus operator.

Thanks for the feedback, I'll change the code to use a string array since I liked that solution more anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I used the template literal but the result ended up being pretty messy (had to create a regex to remove trailing spaces since all lines were indented and account also for the blank lines)

My favorite way is to use a template literal but put the text all the way to the left. It doesn't have an extra runtime cost like the other options it is the easiest to read IMO:

// The initial backslash escapes the first \n
const text = `\ 
We are working hard to make this a great tool, but there's still a pretty good
chance you'll encounter missing pieces, rough edges, performance issues and even,
god forbid, bugs 🐞.`;

Does it look ugly because it's all the way to the left? I guess so, but less ugly than other options to me. If it's really too ugly we could indent it and run code after to dedent it, but that seems like a waste of the user's cpu.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My favorite way is to use a template literal but put the text all the way to the left.

It's funny because I did also that at some point but then thought "better change it, I doubt it meets their coding style". 😂

I like your pragmatic performance-oriented reasoning btw, I also try to think about saving CPU clocks in anything I do.

@tsuf239
Copy link
Collaborator

tsuf239 commented Jun 26, 2023

Also, the build isn't passing because of the snapshots probably- as it is always the first run for github actions machines.
We might need to find a way to bypass it 🤔 can we maybe configure them to have this file on setup?

@garysassano garysassano force-pushed the cli-disclaimer branch 2 times, most recently from a032dd2 to 62c8f6d Compare June 30, 2023 05:03
@hasanaburayyan
Copy link
Contributor

Closing this as #3185 is absorbing the work

@garysassano garysassano deleted the cli-disclaimer branch August 18, 2023 23:26
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.

Print pre-release +analytics disclaimer on first run of Wing CLI
4 participants