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

Update more CLI texts #1301

Merged
merged 7 commits into from
Oct 2, 2024
Merged

Update more CLI texts #1301

merged 7 commits into from
Oct 2, 2024

Conversation

chgeo
Copy link
Member

@chgeo chgeo commented Oct 1, 2024

Make #1294 more generic and update more CLI texts.

  • Use the <pre class="log"> approach instead of log code fence as it allows us to style the output more.
  • Convert known terminal escape sequences to formats (mainly <em>, <i>, <strong>).
  • Remove the versions numbers from the output as these would produce diffs for each new version, although the texts have not changed.
  • Fix the workflow and produce a PR with a stable title. Parallel PRs seem unnecessary.

This also gets rid of the custom renovate config to bump inner-md versions and the corresponding PRs.

@chgeo chgeo force-pushed the cli-out branch 15 times, most recently from 9539388 to a6f23dd Compare October 1, 2024 21:32
@chgeo chgeo changed the title Generic CLI output grabbing Update more CLI texts Oct 1, 2024
@renejeglinsky
Copy link
Contributor

I'd like to have a diff even if only the version number changed and not the text. Why not keep the versions up to date?

@daogrady
Copy link
Contributor

daogrady commented Oct 2, 2024

I'd like to have a diff even if only the version number changed and not the text. Why not keep the versions up to date?

Was also my first impulse, but I see that the version number is no longer part of the output, so there wouldn't be any confusion for the user if they're looking at outdated versions. Plus if the version was still part of the output, this workflow would produce PRs for every patch version of our tools.
So I think this change makes sense, but if we do want to include the version number again, we should at least restrict it to major.minor and exclude .patch.

@daogrady daogrady self-requested a review October 2, 2024 06:27
Copy link
Contributor

@daogrady daogrady left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for making this generalisation!

I played around with the script and it looks fine to me. My gripe with contributing workflows is that they're hard to test, so I'd say merge and see what happens1? 🥲

Footnotes

  1. unless the other reviewers have change-requests, ofc.

@chgeo
Copy link
Member Author

chgeo commented Oct 2, 2024

Was also my first impulse, but I see that the version number is no longer part of the output, so there wouldn't be any confusion for the user if they're looking at outdated versions.

Do you think the user is interested in the exact version? I doubt that and think they just assume they're looking at the latest text. Plus, the former output was a bit distracting like

> @sap/[email protected] cds --help

while this is what users know (and type):

> cds --help

Plus if the version was still part of the output, this workflow would produce PRs for every patch version of our tools.

This is what happens today through a custom renovate config, which turned out to produce quite a lot of rather dump 'just bump the version' PRs (example). This is what I want to reduce.

@chgeo
Copy link
Member Author

chgeo commented Oct 2, 2024

My gripe with contributing workflows is that they're hard to test, so I'd say merge and see what happens? 🥲

I already tested the workflow. Turns out you can trigger workflows from non-main branches through the gh command line tool.

gh workflow run 'PR Latest cds-typer Output' --ref cli-out

What is strange though that I sometimes needed to use the old name PR Latest cds-typer Output while in some instances I could use the new name Update CLI Texts. Looks like a GH bug or so. See the latest run.

@daogrady
Copy link
Contributor

daogrady commented Oct 2, 2024

Sorry, my comment was a bit vague. I initially had the same reaction as René, but after some consideration I came to the same conclusions as you did. So I think removing the version from the output is fine and even if we should reintroduce them, we should at least leave out the patch version.

@daogrady
Copy link
Contributor

daogrady commented Oct 2, 2024

Turns out you can trigger workflows from non-main branches through the gh command line tool.

gh workflow run 'PR Latest cds-typer Output' --ref cli-out

Awesome! 😮

@chgeo
Copy link
Member Author

chgeo commented Oct 2, 2024

I'd like to have a diff even if only the version number changed and not the text. Why not keep the versions up to date?

In the files where versions explicitly appear (like @sap/cds: 8.3.0), we do keep them up to date now, just not by single PRs from renovate but by only one for all texts.

The other thing @daogrady and I discussed was whether they should appear in command prompts like [email protected] --help, and here I'd say that's too confusing to read.

@chgeo
Copy link
Member Author

chgeo commented Oct 2, 2024

Can we merge this?

@chgeo chgeo requested a review from smahati as a code owner October 2, 2024 08:24
@chgeo chgeo enabled auto-merge (squash) October 2, 2024 08:25
@chgeo chgeo disabled auto-merge October 2, 2024 08:29
@chgeo chgeo merged commit e34a80c into main Oct 2, 2024
5 checks passed
@chgeo chgeo deleted the cli-out branch October 2, 2024 08:29
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.

4 participants