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

Correctly display connections to lambdas #7550

Merged
merged 7 commits into from
Aug 11, 2023

Conversation

vitvakatu
Copy link
Contributor

Pull Request Description

Closes #7261

It's impossible to connect to the lambda arguments, but they are displayed as in code, and the correct span tree is generated for the lambda body. (hence you can connect to items inside)

Kapture.2023-08-10.at.16.35.32.mp4

Important Notes

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,
    and
    Rust
    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.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

Copy link
Contributor

@farmaazon farmaazon left a comment

Choose a reason for hiding this comment

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

Approve to not block it, but I strongly recommend to not build span tree only to count AST node length.

.add_leaf(4, 1, node::Kind::argument(), InfixCrumb::RightOperand)
.add_empty_child(5, Append)
.done()
.done()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add also a test case for lambda like a -> b -> a + b

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 845 to 849
SpanSeed::Child(ast::SpanSeedChild { node }) => {
let kind = node::Kind::argument().with_removable(false);
let node = node.generate_node(kind, context);
node.map(|node| node.size.as_usize())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand correctly that we generate node only to discard it and read its size?

Are there no better options for measuring new AST node size? Like repr_len or char_count in the old AST? I believe generating span tree does much more things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, repr_len is better here, thanks for noting. I changed the code.

Comment on lines +823 to +824
/// Both the lambda argument and the `->` token, as a single [`node::Kind::Token`] node.
child: node::Child,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little afraid of the consequences of folding all arguments and the arrow together into a single token. That means we won't be able to create any useful widgets there, like the ability to add more arguments or apply custom styling. But I guess we can expand this logic once we need it, and likely the span-tree will be removed at this point anyway. So I think it's fine as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keep in mind that we were not planning to support connections to lambdas at all, so yes, let's think about it when the time comes :)

@Frizi
Copy link
Contributor

Frizi commented Aug 11, 2023

🌈 QA Passed 🟢

Nice work!

@vitvakatu vitvakatu added the CI: Ready to merge This PR is eligible for automatic merge label Aug 11, 2023
@mergify mergify bot merged commit 9f4a5f9 into develop Aug 11, 2023
27 checks passed
@mergify mergify bot deleted the wip/vitvakatu/lambda-edges-7261 branch August 11, 2023 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connection to the lambdas are appearing on the left side on the node
3 participants