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: copy oclif/core ux methods #537

Merged
merged 30 commits into from
Jun 5, 2024
Merged

feat: copy oclif/core ux methods #537

merged 30 commits into from
Jun 5, 2024

Conversation

mdonnalley
Copy link
Contributor

@oclif/core is slimming down the ux module: oclif/core#1056

As a result, we're going to copy over the functionality that sf-plugins-core relies on. Specifically:

  • styledObject
  • styledJSON
  • table
  • url
  • progress

This serves two purposes:

  • easier upgrade path for sf-plugins-core to update the new major of oclif/core
  • we can delay switching to new libraries for this functionality since that will have customer impact (i.e. tables look different, etc.)

@W-15470636@

Copy link
Contributor

@mshanemc mshanemc left a comment

Choose a reason for hiding this comment

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

approved (seems like goal is lift + shift, no breaking changes).

It's worth a bit of github searching to see if anyone's ever used some of the table features mentioned. If not, maybe we can drop some complexity.

src/ux/table.ts Outdated
return width;
}

const stdtermwidth = Number.parseInt(process.env.OCLIF_COLUMNS!, 10) || termwidth(process.stdout);
Copy link
Contributor

Choose a reason for hiding this comment

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

intentionally using || for falsy 0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Number.parseInt(undefined, 10) equals NaN and NaN ?? true returns NaN so we need ||

❯ node
Welcome to Node.js v20.11.0.
Type ".help" for more information.
> Number.parseInt(undefined, 10)
NaN
> NaN ?? true
NaN
> NaN || true
true

test/integration/ux/table.integration.ts Show resolved Hide resolved
src/ux/table.ts Outdated Show resolved Hide resolved
src/ux/table.ts Outdated Show resolved Hide resolved
src/ux/table.ts Show resolved Hide resolved
src/ux/styledJSON.ts Outdated Show resolved Hide resolved
@mdonnalley mdonnalley requested a review from a team as a code owner May 30, 2024 14:41
src/ux/table.ts Outdated Show resolved Hide resolved
src/ux/table.ts Outdated Show resolved Hide resolved
src/ux/table.ts Outdated Show resolved Hide resolved
src/ux/table.ts Show resolved Hide resolved
src/ux/table.ts Show resolved Hide resolved
Comment on lines +231 to +236
let numOfLines = 1;
for (const col of columns) {
const d = row[col.key] as string;
const lines = d.split('\n').length;
if (lines > numOfLines) numOfLines = lines;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let numOfLines = 1;
for (const col of columns) {
const d = row[col.key] as string;
const lines = d.split('\n').length;
if (lines > numOfLines) numOfLines = lines;
}
const numOfLines = Math.max(1, ...columns.map((c) => (row[c.key] as string).split('\n').length));

src/ux/table.ts Outdated
Comment on lines 39 to 40
const stdtermwidth = Number.parseInt(process.env.OCLIF_COLUMNS!, 10) || termwidth(process.stdout);

Copy link
Contributor

Choose a reason for hiding this comment

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

checking the type first instead of relying on ! and NaN's behavior is clearer

Suggested change
const stdtermwidth = Number.parseInt(process.env.OCLIF_COLUMNS!, 10) || termwidth(process.stdout);
const stdtermwidth =
typeof process.env.OCLIF_COLUMNS === 'string' ? Number.parseInt(process.env.OCLIF_COLUMNS, 10) : termwidth(process.stdout);

@mshanemc
Copy link
Contributor

mshanemc commented Jun 5, 2024

QA:

✅ used existing table from plugin-marketplace
✅ this.url on each pluigin's homepage
✅ this.url doesn't print in --json mode
✅ use each of StndardColors and see how they look

📓 passing no data to this.table will print headers/horizontal lines only. If we did want smarter default behaviors for table, this might be a good time since it only affects sf plugins. Or maybe we could do it anytime since it's human-only UI and we have a good policy around that

✅ styled object with a flat object gives a nice table of key/value output

name:        sfdx-git-delta
description: Generate the sfdx content in source format and destructive change from two git commits
homepage:    https://github.com/scolladon/sfdx-git-delta#readme
downloads:   63935
published:   2024-05-16

styledObject for an array of objects...does that look right? Or is it not meant to take arrays?
Screenshot 2024-06-05 at 1 05 59 PM

@mdonnalley
Copy link
Contributor Author

❓ styledObject for an array of objects...does that look right? Or is it not meant to take arrays?

Yes, I think that's right. Here's the old oclif/core version:

ux.styledObject([
  {description: 'foo description', name: 'foo', value: 'foo'},
  {description: 'foo description', name: 'bar', value: 'bar'},
])
0: description: 'foo description', name: 'foo', value: 'foo'
1: description: 'foo description', name: 'bar', value: 'bar'

@mshanemc
Copy link
Contributor

mshanemc commented Jun 5, 2024

table QA

✅ sort works
📓 I can't see any impact from OCLIF_COLUMNS 🤷🏻
✅ title shows, adds all the ugly ASCII characters
✅ no-header works
✅ extended columns only show up when extended is true

@mshanemc mshanemc merged commit 97f5584 into main Jun 5, 2024
13 of 47 checks passed
@mshanemc mshanemc deleted the mdonnalley/ux branch June 5, 2024 20:18
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