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

Named arguments support in IDE #5774

Merged
merged 11 commits into from
Mar 9, 2023
Merged

Conversation

Frizi
Copy link
Contributor

@Frizi Frizi commented Feb 27, 2023

Pull Request Description

Added support for named arguments in IDE.

named-arg.mp4

Named arguments are now recognized in node expressions. The function argument placeholders are rendered around series of named arguments. Insertion and deletion of arguments either by connection dragging or by widget selection will cause arguments around to be rewritten into appropriate form, such that the meaning of the expression doesn't change. We no longer need to introduce any wildcards (_) in argument positions when editing an argument list of a resolved method.
image

For unresolved function calls, the old behaviour remains, as we don't have data about argument names or their desired order.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@Frizi Frizi force-pushed the wip/frizi/ide-named-arguments branch from b438276 to ac1c90a Compare March 7, 2023 21:44
@Frizi Frizi linked an issue Mar 7, 2023 that may be closed by this pull request
@Frizi Frizi force-pushed the wip/frizi/ide-named-arguments branch from 1c1b545 to 5568c13 Compare March 8, 2023 09:00
@Frizi Frizi marked this pull request as ready for review March 8, 2023 10:46
Copy link
Member

@wdanilo wdanilo left a comment

Choose a reason for hiding this comment

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

The code is easy to read, has beautiful comments with explanations and I do not have any comments to the code really. Needs QA, but beside that, it's very nice!

app/gui/language/span-tree/src/generate.rs Show resolved Hide resolved
@vitvakatu
Copy link
Contributor

QA report

I noticed two issues that may or may not be regressions.

Colors of edges seem to be random

Notice the color of the this argument changes when I connect one of the inputs.

2023-03-09.00-39-48.mp4

Clicking on the output port creates an edge

I connected the first edge, then I clicked on another node's output port – and you can see what happened on the video. After that, clicking on output ports and dragging edges produces weird results without much pattern. Sometimes I even couldn't drop the connection without connecting it to anything. It happens even after reopening the project.

2023-03-09.00-43-18.mp4

Config

Frontend:
    Version:  2023.1.1-dev
    Build:    333e5a7657a200d5b75956c7098908f2022228cd
    Electron: 23.0.0
    Chrome:   110.0.5481.77

Backend:
    Enso Project Manager
    Version:    2023.1.1-nightly.2023.3.4
    Built with: scala-2.13.8 for GraalVM 22.3.1
    Built from: develop @ f64edd806d4de7e71e6b1fea41a13ac189ebfadd
    Built on:   Mac OS X (amd64)

@wdanilo
Copy link
Member

wdanilo commented Mar 8, 2023

@vitvakatu amazing job with finding the issues!

@Frizi
Copy link
Contributor Author

Frizi commented Mar 9, 2023

@vitvakatu I have reproduced both of those issues on current develop. Those are not introduced in this PR.

port-click-edge-bug.mp4

The reproduction indeed seems a bit random. The edge coloring bug is fairly easy to trigger. The one when clicks cause edges to be modified or added required some more fiddling. I randomly connected and moved things around. After a while, clicking on ports started causing weird behaviours.

Copy link
Contributor

@vitvakatu vitvakatu left a comment

Choose a reason for hiding this comment

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

QA accepted. The spotted regressions will be reported as issues by me.

@Frizi Frizi added the CI: Ready to merge This PR is eligible for automatic merge label Mar 9, 2023
@mergify mergify bot merged commit 73487ad into develop Mar 9, 2023
@mergify mergify bot deleted the wip/frizi/ide-named-arguments branch March 9, 2023 21:23
@unfurl-links unfurl-links bot mentioned this pull request Mar 10, 2023
4 tasks
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
3 participants