Skip to content

Refactor: propagate errors in typed_ast#7

Merged
schurhammer merged 8 commits into
schurhammer:mainfrom
gertvv:feature/lib-typed_ast
Feb 26, 2026
Merged

Refactor: propagate errors in typed_ast#7
schurhammer merged 8 commits into
schurhammer:mainfrom
gertvv:feature/lib-typed_ast

Conversation

@gertvv
Copy link
Copy Markdown
Contributor

@gertvv gertvv commented Feb 25, 2026

For issue #6.

This PR replaces most instances of let assert and panic with explicit Result(a, Error) returns. There are two issues of code style that I noticed halfway through and I've adjusted parts of the code I touched to the style I started out with, but let me know if you'd like them changed to the alternative:

  1. I prefer to qualify result.try - this feels more natural/readable to me.
  2. I'm used to using result.map when it makes sense. You seem to typically use result.try and return an explicit Ok(thing) at the end. I see advantages to the latter style, in particular during refactoring and in longer code blocks.

Comment thread src/gig/typed_ast.gleam
@schurhammer schurhammer marked this pull request as draft February 25, 2026 09:08
@gertvv
Copy link
Copy Markdown
Contributor Author

gertvv commented Feb 25, 2026

I'm not convinced the fix I pushed will make the tests pass. The tests pass for me locally with or without the fix. Any ideas?

@schurhammer
Copy link
Copy Markdown
Owner

I'm not convinced the fix I pushed will make the tests pass. The tests pass for me locally with or without the fix. Any ideas?

The tests failed on compiling gig itself to binary - I assume locally you already had the binary available, so this part didn't run. Try deleting the binary and c file between test runs.

Anyway, tests passed :)

@gertvv gertvv marked this pull request as ready for review February 25, 2026 19:45
Comment thread src/gig/compiler.gleam Outdated
Comment thread src/gig/typed_ast.gleam Outdated
Comment thread src/gig/typed_ast.gleam Outdated
Comment thread src/gig/typed_ast.gleam Outdated
Comment thread src/gig/typed_ast.gleam Outdated
Comment thread src/gig/typed_ast.gleam Outdated
Comment thread src/gig/typed_ast.gleam Outdated
@schurhammer
Copy link
Copy Markdown
Owner

schurhammer commented Feb 26, 2026

Looks good, I've left some comments about your TODOs

edit: github doesn't show the line below the TODO comment.. thats annoying

@gertvv
Copy link
Copy Markdown
Contributor Author

gertvv commented Feb 26, 2026

Thanks, all comments addressed.

@schurhammer
Copy link
Copy Markdown
Owner

Awesome, I will merge now. Thank you 💜

@schurhammer schurhammer merged commit 2f4b6d6 into schurhammer:main Feb 26, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants