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

Add Crystal::Repl#parse_and_interpret #14138

Merged
merged 3 commits into from
Jan 2, 2024

Conversation

bcardiff
Copy link
Member

This PR adds a Crystal::Repl#parse_and_interpret method that is convenient to use the repl interpreter as library.

The current interpreter api seems lower level . The repl is, as expected, tight to getting the input from stdin.

The added spec shows how the repl can be instantiated and consumed as a library.

Comment on lines 70 to 77
def runtime_type : Crystal::Type
if type.is_a?(Crystal::UnionType)
type_id = @pointer.as(Int32*).value
context.type_from_id(type_id)
else
type
end
end
Copy link
Member

Choose a reason for hiding this comment

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

chore: This feels like it should be introduced as a separate feature. It's not directly related to #parse_and_interpret. The specs should probably work fine without it, but we could also stack PRs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup. I aimed to that initially but then I remembered the one repl with prelude restriction and somehow push them together.

I'll send another PR for that.

In it we could use this to show type information as part of the repl: eg 1 :: Int32 <: (String | Int32) when instructed, but I'm not fully sure about the syntax. Or we can leave that usage for later also.

Comment on lines 45 to 46
record EvalSuccess, value : Value, warnings : WarningCollection
record EvalParserError, warnings : WarningCollection
Copy link
Member

Choose a reason for hiding this comment

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

question: Have you considered merging these two types as EvalResult? Their shape is similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Using a nilable value would be enough

@bcardiff
Copy link
Member Author

I'm not sure if the CI failures are noise or actual failures. I am not familiar with latest changes on it and the missing symbol puzzles me.

@HertzDevil
Copy link
Contributor

Either the spec should be moved under spec/compiler/interpreter/, or {% skip_file if flag?(:without_interpreter) %} should be added to the beginning of the file

@bcardiff
Copy link
Member Author

CI is happy. Thanks @HertzDevil

@straight-shoota straight-shoota added this to the 1.11.0 milestone Jan 2, 2024
@bcardiff bcardiff merged commit 316422f into crystal-lang:master Jan 2, 2024
56 checks passed
@straight-shoota straight-shoota changed the title Add Crystal::Repl#parse_and_interpret Add Crystal::Repl#parse_and_interpret Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants