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

Show only fields from current atom constructor in the debugger #11217

Merged
merged 27 commits into from
Oct 15, 2024

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Sep 30, 2024

Fixes #10675

Pull Request Description

Debugger shows only fields of the current atom constructor: (internal members shown in gray, "public" members shown in bold purple)
image

(Note that static_method is not displayed as a member of My_Type - not in scope of this PR)

Important Notes

The interop contract for Atom is changed as follows:

  • Members are all methods and fields of the current constructor.
  • All methods are internal members.
  • If the constructor is project-private, fields are internal members.
  • All the members are both readable, and invocable.
    • Fields are field getters, that is, they are just methods.
  • Fields are not invocable
  • Constructors and static methods are not members of an atom. They should be members of the type.
  • Note that methods used to be atom members before Atom constructors can be private #9692

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.

@Akirathan Akirathan added the CI: No changelog needed Do not require a changelog entry for this PR. label Sep 30, 2024
@Akirathan Akirathan self-assigned this Sep 30, 2024
@Akirathan Akirathan changed the title Enable ignored tests Show only fields from current atom constructor in the debugger Oct 4, 2024
@Akirathan Akirathan added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Oct 6, 2024
@Akirathan
Copy link
Member Author

Request for comments @radeusgd @jdunkerley @GregoryTravis @JaroslavTulach @hubertp @4e6 and anybody else using Chrome Inspector for debugging.

Atom now has methods as members. This means that all the methods will be (alphabetically) displayed in the debugger:
inspector-all-methods

An advantage of it is that, inside the console, tab completion works:
inspector_tab_completion

There may be a way to hide the methods from the debugger, display only fields, with the tab completion in the console still working. What are your opinions? Is the list of methods (displayed alphabetically) on the atom something that you consider useful? Let's vote:

  • Thumbs up: it is useful, display methods, fields and have tab completion for methods and fields in console
  • Thumbs down: it is not useful. Diplay only fields and have tab completion for methods and fields in console.

@Akirathan Akirathan marked this pull request as ready for review October 10, 2024 17:09
@Akirathan Akirathan requested review from 4e6 and hubertp as code owners October 10, 2024 17:09
Return list of public methods
@radeusgd
Copy link
Member

If we can still get tab completions for methods without displaying them, IMO that's best.

Displaying methods, where their representation is just f () is not that helpful and it can obscure the view of fields the instance has - whose values may actually aid in debugging.

It's nice to see method names, but if as you say - tab completions still help with those - that will be IMO enough.

@Akirathan
Copy link
Member Author

The current situation is as follows:

  • All methods are internal members - they are displayed by the inspector in gray color.
  • Only fields from public constructor is a proper member - displayed in bold purple by the inspector.
  • Table completion works only on public members, not for internal members.

image

I don't know how to convince the inspector to not display internal members, and to provide proper tab completion. However, as you can see from the screenshot above, the situation is at least better than before - all methods are in gray and so you can immediately see the "important stuff". Unless there are strong opinions against, I would like to merge this PR and optionally create new tickets to improve the debugging experience. I believe the promised fix of this PR is delivered.

@radeusgd
Copy link
Member

Sounds good.

I need to convince myself to use the debugger more, since it's getting such cool new features 😃

They are internal, other languages should deal with that in their own way.
Copy link
Contributor

@GregoryTravis GregoryTravis left a comment

Choose a reason for hiding this comment

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

Approving .enso changes.

}
}
return false;
// All methods are internal.
Copy link
Contributor

Choose a reason for hiding this comment

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

That appears to contradict InteropLibrary.isMemberInternal which says the default is false?

Copy link
Member Author

Choose a reason for hiding this comment

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

This method returns false only for fields of a public constructor. All the other members, including all methods, are considered internal. That is, fields are "public members", methods are internal members. This is compatible with what we used to have before #9692.

@Akirathan Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label Oct 14, 2024
@mergify mergify bot merged commit 4a2e522 into develop Oct 15, 2024
42 checks passed
@mergify mergify bot deleted the wip/akirathan/10675-debugger-atom-fields branch October 15, 2024 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Debugger doesn't show atom fields when there are multiple constructors
5 participants