Skip to content

Conversation

baraich
Copy link

@baraich baraich commented Sep 19, 2025

When trying to specify return types for a function using a :, it now displays a better error message.

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you! Could you use an if let Some with an early return for this please, similar to the error above for FunctionDefinitionAngleGenerics.

We'll need tests to be added too. The ones for FunctionDefinitionAngleGenerics would be a good example for what they would look like, you can see them in this PR #4818

@lpil lpil marked this pull request as draft September 19, 2025 11:09
@baraich
Copy link
Author

baraich commented Sep 19, 2025

@lpil, I need your help in writing tests for these changes. I have written the following test; however, it is failing.

#[test]
fn wrong_function_return_type_declaration_using_colon_instead_of_right_arrow() {
    assert_module_error!(
        r#"
pub fn main(): Nil {}
        "#
    );
}

@GearsDatapacks
Copy link
Member

You probably need to accept the snapshot. We use snapshot testing in the Gleam compiler, so once you run the test, you need to run cargo insta review. Then you look at the output (in this case the error message) and you decide whether or not it is correct. See docs/compiler/README.md for more information

@baraich baraich marked this pull request as ready for review September 20, 2025 02:59
@baraich
Copy link
Author

baraich commented Sep 21, 2025

@GearsDatapacks, please review the pull request.

Copy link
Member

@GearsDatapacks GearsDatapacks left a comment

Choose a reason for hiding this comment

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

Looks good! One tiny note

@baraich
Copy link
Author

baraich commented Sep 21, 2025

@GearsDatapacks, I have updated the code with the suggested change.

Copy link
Member

@GearsDatapacks GearsDatapacks left a comment

Choose a reason for hiding this comment

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

Looks good to me! Need Louis to review it before merging though

@baraich
Copy link
Author

baraich commented Sep 22, 2025

@lpil Please review the pull request.

@baraich baraich requested a review from lpil September 22, 2025 07:38
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Lovely work! Thank you very much!

I've left one tiny note inline, and would you mind updating the changelog too please 🙏

@lpil
Copy link
Member

lpil commented Sep 22, 2025

Ah! The build is failing because of a CI problem. If you rebase on main to the latest code changes then it should be fixed.

@baraich
Copy link
Author

baraich commented Sep 22, 2025

@lpil, I will rebase it on the main branch and update the CHANGELOG.md file.

@baraich baraich requested a review from lpil September 23, 2025 06:39
@baraich
Copy link
Author

baraich commented Sep 23, 2025

@lpil, Please review the pull request.

@lpil
Copy link
Member

lpil commented Sep 23, 2025

You've done a merge commit rather than a rebase, so the build is failing as we do not permit them. Could you rebase on main please 🙏 That will remove the merge commit for you.

@baraich
Copy link
Author

baraich commented Sep 23, 2025

@lpil, I have no idea how to perform a rebase. Now, how do I revert the accidental merge? I just ran git pull --rebase on the main branch.

@inoas
Copy link
Contributor

inoas commented Sep 24, 2025

@baraich I think we could possibly help you out on community discord.

@baraich
Copy link
Author

baraich commented Sep 24, 2025

@inoas, is there an invite link?

@inoas
Copy link
Contributor

inoas commented Sep 24, 2025

@inoas, is there an invite link?

https://gleam.run/community/

@ankddev
Copy link
Contributor

ankddev commented Sep 24, 2025

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.

5 participants