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

Port graph editor to new AST #4113

Merged
merged 97 commits into from
Feb 10, 2023
Merged

Port graph editor to new AST #4113

merged 97 commits into from
Feb 10, 2023

Conversation

kazcw
Copy link
Contributor

@kazcw kazcw commented Feb 1, 2023

Pull Request Description

Use the Rust parser rather than the Scala parser to parse Enso code in the IDE.

Implements:

There is additional functionality needed before the transition is fully-completed, however I think it's time for this to see review and testing, so I've opened separate issues. In rough order of urgency (these issues are also linked from the corresponding disabled tests):

Important Notes

The implementation is based partly on translation, and partly on new analysis. Method- and operator-related shapes are translated to the old Ast variants, so that all the analysis applied to them doesn't need to be ported at this time. Everything else (mostly "macros" in the old AST) is implemented with new analysis.

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.

@kazcw kazcw added the CI: No changelog needed Do not require a changelog entry for this PR. label Feb 1, 2023
@kazcw kazcw self-assigned this Feb 1, 2023
@kazcw kazcw force-pushed the wip/kw/parser/port-graph-editor branch from b83f430 to 740d9c1 Compare February 1, 2023 21: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.

I don't have words to tell you how happy I am it's here. Before merge please:

  1. Be sure someone did Q/A of it. Ask about it on Discord.
  2. Add doc comments where necessary. There are some code pieces not commented at all.

app/gui/language/ast-parser/Cargo.toml Outdated Show resolved Hide resolved
app/gui/language/ast-parser/src/translation.rs Outdated Show resolved Hide resolved
app/gui/language/ast-parser/src/translation.rs Outdated Show resolved Hide resolved
app/gui/language/ast-parser/src/translation.rs Outdated Show resolved Hide resolved
app/gui/language/ast-parser/src/translation.rs Outdated Show resolved Hide resolved
@kazcw
Copy link
Contributor Author

kazcw commented Feb 8, 2023

@JaroslavTulach or @hubertp -- could you review the backend change? The parser now rejects import statements that have hiding without all (this combination has not been in use since a libs cleanup).

@enso-bot
Copy link

enso-bot bot commented Feb 9, 2023

Keziah Wesley reports a new STANDUP for yesterday (2023-02-07):

Progress: Removed unwraps and handled any possible or maybe-possible Tree. Implemented full comment representation (#5573). It should be finished by 2023-02-08.

Next Day: Next day I will be working on the #4113 task. Implement any review changes. Work on the remaining high-priority follow-up issue (#5571).

@enso-bot
Copy link

enso-bot bot commented Feb 9, 2023

Keziah Wesley reports a new STANDUP for today (2023-02-08):

Progress: Implemented review changes (docs); fixed issues I found in additional testing.
Debugged and fixed a bug in handling a syntax error in the parser (#5056). It should be finished by 2023-02-08.

Next Day: Start working on performance.

@sylwiabr
Copy link
Member

sylwiabr commented Feb 9, 2023

@JaroslavTulach @hubertp please review the engine changes in the PR - it would be great if we could land it ASAP ant test the effects of replacing parser in IDE

@vitvakatu vitvakatu self-assigned this Feb 9, 2023
Copy link
Contributor

@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.

Engine change LGTM

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 acceptance passed for 197ddb13fb698a68f9061322735a66b1efeb8bbb and Engine version 2023.1.1-nightly.2023.2.9. No issues related to this PR were were found though I found a couple of issues on develop.

@kazcw kazcw added the CI: Ready to merge This PR is eligible for automatic merge label Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
5 participants