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.

Sign up for GitHub

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

Jump to bottom

feat(ext/console): Only right-align integers in console.table() #17389

Merged
merged 4 commits into from
Feb 8, 2023
Merged

feat(ext/console): Only right-align integers in console.table() #17389

merged 4 commits into from
Feb 8, 2023

Conversation

waldyrious
Copy link
Contributor

@waldyrious waldyrious commented Jan 13, 2023 •
edited
Loading

This PR changes console.table() to only right-align columns when their contents are integers (rather than any numbers, as is currently the case). This was proposed in this comment on #11294. A new test is added to validate this behavior.

The goal is to change from this:

┌───────┬───────┬────────┐
│ (idx) │ foo   │ bar    │
├───────┼───────┼────────┤
│     0 │   456 │    1.1 │
│     1 │    32 │      2 │
│     2 │       │ 3.1415 │
└───────┴───────┴────────┘

to this:

┌───────┬───────┬────────┐
│ (idx) │ foo   │ bar    │
├───────┼───────┼────────┤
│     0 │   456 │ 123.45 │
│     1 │    32 │ 2      │
│     2 │       │ 3.1415 │
└───────┴───────┴────────┘

(Of course, the ideal would be to align numbers regardless of how many decimal places they have, i.e.:

┌───────┬───────┬──────────┐
│ (idx) │ foo   │ bar      │
├───────┼───────┼──────────┤
│     0 │   456 │ 123.45   │
│     1 │    32 │   2      │
│     2 │       │   3.1415 │
└───────┴───────┴──────────┘

...but I don't know how to go about doing that 😅)

Follow-up of #11295 and #11748.

I also tweaked formatting of console.table() tests (in a separate commit), to make the table format more evident in the source code of the tests.

Note: I'm opening as draft because I can't get the tests to pass — something in the logic to detect whether an entire column consists only of integers seems to be failing, but I'm not sure what. Any assistance would greatly appreciated!

@CLAassistant
Copy link

CLAassistant commented Jan 13, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@waldyrious waldyrious force-pushed the console-table-number-alignment branch from f450014 to 6f63857 Compare January 13, 2023 12:41
@waldyrious
Copy link
Contributor Author

Here's a screenshot of the failing (newly added) test:

image

I'm not sure why the "expected" part is printed like that — it is correctly aligned in the source code...

Btw, this change also affects other tests that should not have been affected, e.g.

image

@bartlomieju
Copy link
Member

@kt3k please review this PR

@bartlomieju bartlomieju changed the title Only right-align integers in console.table() feat(ext/console): Only right-align integers in console.table() Feb 4, 2023
@bartlomieju bartlomieju added this to the 1.31 milestone Feb 4, 2023
@kt3k
Copy link
Member

kt3k commented Feb 7, 2023

The diff looks good to me.

@waldyrious

Here's a screenshot of the failing (newly added) test:

I don't see the failing test cases. Have you fixed them already?

@waldyrious
Copy link
Contributor Author

I don't see the failing test cases. Have you fixed them already?

No, I didn't make changes to this branch since my last comment. Maybe something else changed in the codebase. I'll rebase the branch and try again.

@waldyrious waldyrious force-pushed the console-table-number-alignment branch from 6f63857 to 350ae92 Compare February 8, 2023 00:35
@waldyrious waldyrious force-pushed the console-table-number-alignment branch from 350ae92 to 98d5b35 Compare February 8, 2023 01:25
@waldyrious
Copy link
Contributor Author

waldyrious commented Feb 8, 2023 •
edited
Loading

I've rebased the branch but I still get a test failure. The situation did improve: I now only get a failure in the specific test I added, rather than also in unrelated ones; and the printing of the results in the console now looks OK:

image

...although it still fails, for some reason. It's as if the Number.isInteger() is not kicking in and all values are left-aligned, even though the (idx) and 0 columns should be right-aligned because they consist solely of integers.

For the record, I'm using ./target/debug/deno test --unstable cli/tests/unit/console_test.ts, but the full cargo test results in the same issue.

@waldyrious
Copy link
Contributor Author

Looks like CI is skipping the tests because the PR is marked as draft. I'll mark it as ready for review so that hopefully the same issue can be reproduced in CI.

@waldyrious waldyrious marked this pull request as ready for review February 8, 2023 01:40
@waldyrious waldyrious force-pushed the console-table-number-alignment branch from 98d5b35 to ba3d582 Compare February 8, 2023 01:53
kt3k
kt3k reviewed Feb 8, 2023
View reviewed changes
ext/console/02_console.js Outdated Show resolved Hide resolved
kt3k
kt3k approved these changes Feb 8, 2023
View reviewed changes
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM

@kt3k kt3k merged commit 19543ff into denoland:main Feb 8, 2023
@waldyrious waldyrious deleted the console-table-number-alignment branch February 8, 2023 11:25
sido378 pushed a commit to dynatrace-oss-contrib/deno that referenced this pull request Feb 13, 2023
@waldyrious @sido378
fix(ext/console): Only right-align integers in console.table() (denol…
5ef5ea0 
…and#17389)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kt3k kt3k kt3k approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.31
Development

Successfully merging this pull request may close these issues.
4 participants
@waldyrious @CLAassistant @bartlomieju @kt3k