-
Notifications
You must be signed in to change notification settings - Fork 4
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
Conversation
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.
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); |
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.
intentionally using || for falsy 0
?
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.
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
BREAKING CHANGE: remove csv, json and yaml from table options
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; | ||
} |
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.
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
const stdtermwidth = Number.parseInt(process.env.OCLIF_COLUMNS!, 10) || termwidth(process.stdout); | ||
|
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.
checking the type first instead of relying on !
and NaN's behavior is clearer
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); |
Yes, I think that's right. Here's the old oclif/core version:
|
table QA ✅ sort works |
@oclif/core is slimming down the
ux
module: oclif/core#1056As 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:
@W-15470636@