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

Reducing helper methods in Standard.Base.Meta #12031

Merged
merged 37 commits into from
Jan 22, 2025

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Jan 9, 2025

Pull Request Description

  • reducing the amount of code inside of Meta.enso in preparation of Function metadata call #5226
  • moving Instrumenter into internal packages
  • moving Instrumenter.uuid (used by visualizations) into Runtime.value_for_uuid
  • fixing Meta.meta Cons... behavior for no non-default argument constructors

Important Notes

Checklist

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

  • All code follows the formatting guidelines
  • Unit tests have been written where possible.
  • Signature test for Meta API snapshot written

@JaroslavTulach JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Jan 13, 2025
@JaroslavTulach JaroslavTulach marked this pull request as ready for review January 21, 2025 06:49
final class MetaKindNode extends Node {
final long execute(@AcceptsError Object value) {
if (value instanceof Atom) {
return 2;
Copy link
Member Author

@JaroslavTulach JaroslavTulach Jan 21, 2025

Choose a reason for hiding this comment

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

I expect magic constant complains... but the most important thing is consistency with Enso source code and I am not aware of any other way to enforce it than having some tests (which do exist).

Copy link
Member

Choose a reason for hiding this comment

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

Fair. It is indeed well-tested I think.

I'd just add a comment in both files mentioning that we want to be consistent with the other file - so if any modification has to happen it is easier to find where the relevant parts are.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to risk loosing the green CI status.

Copy link
Member

Choose a reason for hiding this comment

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

Is that really a good reason to submit code with bad formatting to the repo?

  • All code follows the formatting guidelines

This is no longer true.

I know we don't (yet) have an official style guide for Enso but misaligned comments are surely not something we should introduce not-by-mistake.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Jan 21, 2025

There is a failure in test/Table_Tests related to removal of Instrumentor, @radeusgd:

Reason: An unexpected panic was thrown: (No_Such_Method.Error (Unresolved_Symbol.Value UnresolvedSymbol<collapsed>) UnresolvedSymbol<instrument>)
️         at <enso> Table_Aggregate_Auto_Constructor.perform<arg-0>(/Users/runner/work/enso/enso/test/Table_Tests/src/Helpers/Table_Aggregate_Auto_Constructor.enso:50:20-52)
️         at <enso> Table_Aggregate_Auto_Constructor.perform<arg-0>(/Users/runner/work/enso/enso/test/Table_Tests/src/Helpers/Table_Aggregate_Auto_Constructor.enso:51:11-24)
️         at <enso> Table_Aggregate_Auto_Constructor.perform<arg-0>(/Users/runner/work/enso/enso/test/Table_Tests/src/Helpers/Table_Aggregate_Auto_Constructor.enso:52:11-27)
️         at <enso> Table_Aggregate_Auto_Constructor.perform<arg-0>(/Users/runner/work/enso/enso/test/Table_Tests/src/Helpers/Table_Aggregate_Auto_Constructor.enso:53:11-25)
️         at <enso> Table_Aggregate_Auto_Constructor.perform(/Users/runner/work/enso/enso/test/Table_Tests/src/Helpers/Table_Aggregate_Auto_Constructor.enso:54:11-18)
️         at <enso> Auto_Constructor_Instrumentation_Spec.add_specs.Auto_Constructor_Instrumentation_Spec.add_specs<arg-1>(/Users/runner/work/enso/enso/test/Table_Tests/src/Helpers/Auto_Constructor_Instrumentation_Spec.enso:13:20-80)

Shall I create test/Table_Internal_Tests and move the test in there?

Update: 67fcec2 creates test/Table_Internal_Tests.

Copy link
Collaborator

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

Please provide instructions in a more visible location.

test/Examples_Tests/src/Signatures_Spec.enso Outdated Show resolved Hide resolved
@@ -0,0 +1,48 @@
## Enso Signatures 1.0
## module Standard.Base.Meta
Copy link
Member Author

Choose a reason for hiding this comment

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

Snapshot of the Meta API used by the Signature_Spec

@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/MetaHelpers5226 branch from fb3196f to 4daed89 Compare January 22, 2025 07:48
Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

LGTM. Just minor nitpicks.

@@ -346,6 +354,30 @@ add_specs suite_builder =
Meta.get_annotation My_Type .Value "bar" . should_equal Nothing
Meta.get_annotation My_Type .Value "baz" . should_equal (My_Type.Value 1 2 3)

group_builder.specify "atom constructor with suspended evalution" <|
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
group_builder.specify "atom constructor with suspended evalution" <|
group_builder.specify "atom constructor with suspended evaluation" <|

_ ->
Test.fail "Expecting Meta.Constructor, but was: "+m.to_text

group_builder.specify "atom without suspended evalution" <|
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
group_builder.specify "atom without suspended evalution" <|
group_builder.specify "atom without suspended evaluation" <|

if (MethodRootNode.constructorFor(fn) instanceof AtomConstructor atom) {
return atom;
}
if (fn.isThunk()) {
Copy link
Member

Choose a reason for hiding this comment

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

Can there be a nested thunk inside a thunk? If not, I would remove the for loop, and just check if fn.isThunk at the beginning of the method and solve the thunk first.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am working on changing this line in a subsequent PR that is going to introduce Meta.Function.

@JaroslavTulach JaroslavTulach merged commit 1f5ba9d into develop Jan 22, 2025
49 checks passed
@JaroslavTulach JaroslavTulach deleted the wip/jtulach/MetaHelpers5226 branch January 22, 2025 17:35
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants