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

Meta.get_constructor_fields works for Type #5869

Merged
merged 2 commits into from
Mar 13, 2023
Merged

Meta.get_constructor_fields works for Type #5869

merged 2 commits into from
Mar 13, 2023

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Mar 9, 2023 •
edited
Loading

Pull Request Description

Fixes #5805 by returning [] as list of fields of Type.

Important Notes

Type is recognized as Meta.is_atom since #3671. However Type isn't an Atom internally. We have to provide special handling for it where needed.

Checklist

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

  • All code follows the
    Java,
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Mar 9, 2023
@JaroslavTulach JaroslavTulach self-assigned this Mar 9, 2023
@JaroslavTulach JaroslavTulach requested a review from kustosz March 9, 2023 07:40
@JaroslavTulach JaroslavTulach mentioned this pull request Mar 9, 2023
Closed
@radeusgd
Copy link
Member

radeusgd commented Mar 9, 2023

Why do we want to treat Type as an Atom? Why not introduce a Meta.Type? I think it could be quite useful to distinguish Atom instances from their Types. They really are different things in the language and the merging of them looks artificial.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Mar 10, 2023 •
edited
Loading

Only one [FAILED test after bd9916e? In such case we can consider implementing Meta.Type. That will require a bit more work, however.

Why not introduce a Meta.Type?

I guess we should fix the error first, and then start design work on the new Meta.Type API. @radeusgd please discuss this with James and if approved and scheduled for any time soon, report an issue for me to address it.

@enso-bot enso-bot bot mentioned this pull request Mar 10, 2023
Closed
@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/MetaFieldsForType_5805 branch from bd9916e to 0a6ac0a Compare March 10, 2023 04:02
@JaroslavTulach JaroslavTulach requested a review from hubertp March 10, 2023 04:04
@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Mar 13, 2023
radeusgd
radeusgd reviewed Mar 13, 2023
View reviewed changes
test/Tests/src/Semantic/Meta_Spec.enso
Comment on lines +202 to +212
Test.specify "fields of a Type" <|
typ = Boolean

Meta.is_atom typ . should_be_true
meta_typ = Meta.meta typ
meta_typ . should_be_a Meta.Atom
fields = case meta_typ of
Meta.Atom.Value _ -> meta_typ.constructor.fields
_ -> Test.fail "Should be a Meta.Atom.Value: " + meta_typ.to_text

fields . should_equal []
Copy link
Member

Choose a reason for hiding this comment

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

Can we also add a test where typ is not a regular type but a module, like Meta?

As mentioned, this also used to fail on modules and I'd like to ensure it doesn't crash there too.

Looks like the fix should apply there just fine, but good to have a test to avoid regressions.

Copy link
Member Author

Choose a reason for hiding this comment

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

The fallback applies to Module as well: https://github.com/enso-org/enso/pull/5869/files#diff-63d4533fccf2dfeb8e87e9ccba516458b86d82dd6f4ee9d9a4b7cc6dec4e667bR35 - too late to do changes to the #5869 PR, but feel free to add a test in its own PR.

@mergify mergify bot merged commit 888307f into develop Mar 13, 2023
@mergify mergify bot deleted the wip/jtulach/MetaFieldsForType_5805 branch March 13, 2023 09:58
@JaroslavTulach
Copy link
Member Author

Why not introduce a Meta.Type?

@radeusgd please take a look at #5956

@radeusgd
Copy link
Member

Why not introduce a Meta.Type?

@radeusgd please take a look at #5956

I did. And I'm really glad we went this way 😁 Thanks for implementing it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@radeusgd radeusgd radeusgd left review comments

@jdunkerley jdunkerley jdunkerley approved these changes

@hubertp hubertp hubertp approved these changes

@4e6 4e6 Awaiting requested review from 4e6 4e6 is a code owner

@kustosz kustosz Awaiting requested review from kustosz

Assignees

@JaroslavTulach JaroslavTulach

Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Meta.meta returns a broken value for Types
4 participants
@JaroslavTulach @radeusgd @hubertp @jdunkerley