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

Move stdlib to different files #291

Merged
merged 26 commits into from
Jul 16, 2024
Merged

Conversation

Mte90
Copy link
Member

@Mte90 Mte90 commented Jul 9, 2024

Based on #152

My changes:

  • Added the include_dir crate that was missing
  • Did the last reviews in that ticket
  • Updated all the tests also for the new functions
  • Organized in a different way

Closes #152

@Mte90 Mte90 requested review from Ph0enixKM and b1ek July 9, 2024 10:43
@Mte90 Mte90 added the pr/breaking This PR introduces a breaking change label Jul 9, 2024
@b1ek
Copy link
Member

b1ek commented Jul 9, 2024

this will close #122

b1ek
b1ek previously requested changes Jul 9, 2024
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.

i think std/action should be std/http, since action covers pretty much any function in stdlib, but http covers only http related stuff, like download which is the only thing in there

same goes for std/misc tbh, as it only covers env related stuff. it should be std/env

@b1ek b1ek linked an issue Jul 9, 2024 that may be closed by this pull request
Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

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

Some bugs found and requested changes

src/modules/imports/import.rs Outdated Show resolved Hide resolved
src/modules/imports/import.rs Outdated Show resolved Hide resolved
src/modules/imports/import.rs Outdated Show resolved Hide resolved
src/modules/imports/import_string.rs Outdated Show resolved Hide resolved
src/std/array.ab Outdated Show resolved Hide resolved
src/std/array.ab Outdated Show resolved Hide resolved
src/std/fs.ab Outdated Show resolved Hide resolved
src/std/misc.ab Outdated Show resolved Hide resolved
src/std/shell.ab Outdated Show resolved Hide resolved
src/std/math.ab Show resolved Hide resolved
src/std/shell.ab Outdated Show resolved Hide resolved
@Mte90 Mte90 requested review from b1ek and Ph0enixKM July 10, 2024 09:03
@Ph0enixKM
Copy link
Member

Once this gets merged we should update this answer: #151

@Mte90 Mte90 dismissed stale reviews from Ph0enixKM and b1ek July 11, 2024 08:38

done

Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

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

This path is invalid. Can you also recompile install.ab script to see if this is working now?

setup/install.ab Outdated Show resolved Hide resolved
@Ph0enixKM
Copy link
Member

@Mte90 could you resolve conflicts?

@Mte90
Copy link
Member Author

Mte90 commented Jul 15, 2024

The input tests fails but the code didn't changed, also locally it fails.

@Ph0enixKM @b1ek do you have some ideas why?
If I run the test script works and also the bash compiled, so to me there is something wrong as it isn't writing anything on stdin.

@CymDeveloppement
Copy link
Member

The input tests fails but the code didn't changed, also locally it fails.

@Ph0enixKM @b1ek do you have some ideas why? If I run the test script works and also the bash compiled, so to me there is something wrong as it isn't writing anything on stdin.

I don't know why it doesn't work, but i think this test can be in pure amber.

import * from "std"
main {
    unsafe $echo "Amber!" >> /tmp/test_input$
    unsafe $exec 0< /tmp/test_input$
    let name = input("Please enter your name:")
    echo "Hello, " + name
}

this output Please enter your name:Hello, Amber!

@Mte90 Mte90 merged commit 98ca813 into amber-lang:master Jul 16, 2024
1 check passed
@Mte90 Mte90 deleted the move-stdlib-v2 branch July 16, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/breaking This PR introduces a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨ should split "std" into submodules
4 participants