Skip to content

Improve object display#4377

Merged
hansl merged 10 commits intoboa-dev:mainfrom
xbwwj:improve-display
Aug 26, 2025
Merged

Improve object display#4377
hansl merged 10 commits intoboa-dev:mainfrom
xbwwj:improve-display

Conversation

@xbwwj
Copy link
Contributor

@xbwwj xbwwj commented Aug 14, 2025

This Pull Request closes #4371.

It changes the following:

  1. show constructor name when display object

    >> class Foo { a = 1 }
    undefined
    >> const foo = new Foo()
    undefined
    >> foo
    Foo {
       a: 1
    }
    
    // nested works well
    >> class Bar { b = new Foo() }
    undefined
    >> const bar = new Bar()
    undefined
    >> bar
    Bar {
       b: Foo {
           a: 1
        }  // <- indent error here, not brought by this PR
    }
    
    // work with `"Object"` as name
    >> class Object { a = 1 }
    undefined
    >> new Object()
    Object {
       a: 1
    }
    
    // plain does not show constructor name
    >> const plain = { a: 123 }
    undefined
    >> plain
    {
       a: 123
    }
    
    // builtin special classes need further improvement but not in this PR,
    // at least now we know its type
    >> new Float32Array([1,2,3,4,5])
    Float32Array {
    
    }
  2. console.{log,error,...} now use JsValue::display for non string value

    >> console.log(123)
    123
    undefined
    >> console.log("456")
    456
    undefined
    >> console.log({a: 123, b: "456"})
    {
       a: 123,
       b: "456"
    }
    undefined
  3. function and class display [class $name], [Function: $name]

    >> console.log(console.log)
    [Function: log]
    undefined
    >> class Foo {}
    undefined
    >> console.log(Foo)
    [class Foo]
    undefined
    >> console.log(ArrayBuffer)
    [class ArrayBuffer]  // <- FIXME: I'm not able to distinguish callable and constructor with current engine
    undefined               // as custom class `Foo` also satisfies `is_callable`
  4. A flake.nix file that makes it easier for Nix users to setup environment and contribute to the project

@xbwwj xbwwj changed the title Improve display Improve object display Aug 14, 2025
@xbwwj xbwwj marked this pull request as ready for review August 15, 2025 03:58
@xbwwj
Copy link
Contributor Author

xbwwj commented Aug 15, 2025

@jedel1043 I think this PR is ready for review now.

@nekevss nekevss requested a review from a team August 15, 2025 14:53
@nekevss nekevss added the A-API Changes related to public APIs label Aug 15, 2025
@codecov
Copy link

codecov bot commented Aug 15, 2025

Codecov Report

❌ Patch coverage is 40.32258% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.35%. Comparing base (6ddc2b4) to head (3de3fa4).
⚠️ Report is 502 commits behind head on main.

Files with missing lines Patch % Lines
core/engine/src/value/display.rs 35.08% 37 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4377      +/-   ##
==========================================
+ Coverage   47.24%   50.35%   +3.11%     
==========================================
  Files         476      508      +32     
  Lines       46892    50906    +4014     
==========================================
+ Hits        22154    25635    +3481     
- Misses      24738    25271     +533     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@xbwwj
Copy link
Contributor Author

xbwwj commented Aug 15, 2025

I'm not familiar with the codecov dashboard. How can I improve this score, by adding more unit tests?

@Razican
Copy link
Member

Razican commented Aug 15, 2025

I'm not familiar with the codecov dashboard. How can I improve this score, by adding more unit tests?

Codecov results show which lines of code are being run in the tests. It could be that some branches (if-else, match) are only being tested for one of the comparison results, instead of all, so they show in red.

Ideally, we should have specific tests for most of the branches. Either as separate tests or as more complete single tests.

@hansl
Copy link
Contributor

hansl commented Aug 16, 2025

We probably don't want the WASMI code in here. You should open a separate PR with it and remove it from here.

I cannot find do a review soon. I like this PR a lot. Thanks.

@xbwwj
Copy link
Contributor Author

xbwwj commented Aug 16, 2025

Sorry. I tried but still cannot get stated with codecov. There isn't even an entry of core/engine/value/display.rs in codecov link.

I also don't have access to codecov.io for my local code changes. Which local tool do you use, cargo-llvm-cov or tarpaulin? There is no related information in the documentation.

@hansl
Copy link
Contributor

hansl commented Aug 19, 2025

Sorry. You can follow the instructions at https://doc.rust-lang.org/beta/rustc/instrument-coverage.html

I personally use IntelliJ which has excellent coverage support. I know Rust Analyzer for VSCode does too.

Two notes;

  1. let's remove all the Nix mentions and make a new PR with it. Focus PRs on single task/issue/feature.
  2. You will need to run cargo fmt --all to fix the formatting.

@nekevss
Copy link
Member

nekevss commented Aug 19, 2025

I wouldn't worry too much about the coverage either. It is definitely nice to have unit tests, but we typically don't reject PRs based off the codecov report.

@xbwwj xbwwj mentioned this pull request Aug 19, 2025
github-merge-queue bot pushed a commit that referenced this pull request Aug 26, 2025
Nix flake is helpful to Nix users for setting up develop environment.
Nix is a popular package manager on Linux, macOS and Android.

This PR is split from #4377 .
Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

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

This could be improved a bit; indentation issue, a lot of allocations, more tests. But I'll approve now and we can work on fixing it in the code base.

@hansl hansl added this pull request to the merge queue Aug 26, 2025
@hansl
Copy link
Contributor

hansl commented Aug 26, 2025

I really like the change, btw. Sorry I couldn't review faster, I was away.

Merged via the queue into boa-dev:main with commit d565cd8 Aug 26, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-API Changes related to public APIs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve object display

4 participants