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

fix(test): missing function parameters #461

Merged
merged 4 commits into from
Sep 11, 2024

Conversation

Mte90
Copy link
Member

@Mte90 Mte90 commented Sep 5, 2024

I noticed that the tests implemented on #260 are missing in the actual release.

Probably during the various PR and changes they are gone.
This PR will implement back.

@hdwalters
Copy link
Contributor

I also note trailing semicolons on some of the lines. They clearly aren't breaking any of the tests, but I didn't think they were necessary in Amber scripts?

hwalters@Wintermute ~/git/amber/src/tests (master) 
$ rg -l ';\s*$'
cli.rs
errors.rs
extra.rs
formatter.rs
mod.rs
stdlib/abs.ab
stdlib/capitalize.ab
stdlib/ceil.ab
stdlib/char_at.ab
stdlib/floor.ab
stdlib/lpad.ab
stdlib/round.ab
stdlib/rpad.ab
stdlib/slice.ab
stdlib/zfill.ab
stdlib.rs
validity.rs

// Output
// 1100

fun sum_array(a : [Num] = [Num]): Num {
Copy link
Member

Choose a reason for hiding this comment

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

Why is the default value a type? Doesn't it raise any errors while compiling?

Copy link
Member Author

Choose a reason for hiding this comment

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

No doesn't, those are the tests for that feature that were missing after implement the feature.

Copy link
Member

Choose a reason for hiding this comment

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

I still dont understand this. Why do we allow for type to be a default value? What is the usecase?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know, I just migrated the test from the PR that was approved and were missing in the actual codebase.

Copy link
Member

Choose a reason for hiding this comment

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

If this test succeeds, this is worth opening an issue about.

Copy link
Member Author

Choose a reason for hiding this comment

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

tests right now works, if someone want to open a ticket about it to me is fine :-)

@Mte90
Copy link
Member Author

Mte90 commented Sep 6, 2024

I also note trailing semicolons on some of the lines. They clearly aren't breaking any of the tests, but I didn't think they were necessary in Amber scripts?

This are stuff for another discussion but right now there isn't any code style for amber itself.

@Mte90 Mte90 requested a review from hdwalters September 6, 2024 08:46
// Output
// 1000

fun sum_array(a : [Num] = [100,200,300,400]): Num {
Copy link
Member

Choose a reason for hiding this comment

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

im pretty sure its supposed to be a: [Num], not a : [Num]

Copy link
Member Author

Choose a reason for hiding this comment

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

right now the tests are working in this way, maybe is just defining a array with numbers?

Copy link
Member

@b1ek b1ek left a comment

Choose a reason for hiding this comment

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

lgtm, just the code formatting thing

Copy link
Contributor

@hdwalters hdwalters left a comment

Choose a reason for hiding this comment

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

There are still a couple of issues with function_optional_argument_array.ab and function_optional_argument_array_default.ab, where you're echoing the expected value, not the value returned by the function under test. So you're asserting that 1000 equals 1000, which is probably not what you meant.

@Mte90 Mte90 requested review from hdwalters and b1ek September 9, 2024 08:09
@Mte90
Copy link
Member Author

Mte90 commented Sep 9, 2024

Thanks everyone I just imported the test from the PR mentioned and that were missing but is clear that they had some issues and we didn't noticed when we approved it.

Copy link
Contributor

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

@mks-h mks-h removed their request for review September 11, 2024 06:38
@Mte90 Mte90 dismissed KrosFire’s stale review September 11, 2024 08:35

topic for another ticket

@Mte90 Mte90 merged commit c1290df into amber-lang:master Sep 11, 2024
1 check passed
Mte90 added a commit to Mte90/Amber that referenced this pull request Sep 19, 2024
* fix(test): missing function parameters

* Update function_optional_argument_array.ab

* fix(test): after review

* fix(test): review
@Mte90 Mte90 deleted the fix-test-def-func branch September 19, 2024 08:50
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