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.

Sign up for GitHub

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

Jump to bottom

feat(builtin): mv builtin #314

Merged
merged 11 commits into from
Jul 29, 2024
Merged

feat(builtin): mv builtin #314

merged 11 commits into from
Jul 29, 2024
+77 −2

Conversation

MuhamedMagdi
Copy link
Contributor

Ref: #215

Mte90
Mte90 reviewed Jul 17, 2024
View reviewed changes
Mte90
Mte90 reviewed Jul 17, 2024
View reviewed changes
@Mte90 Mte90 requested a review from Ph0enixKM July 17, 2024 09:01
@MuhamedMagdi
fix: run tests in /temp dir
ad832be 
create file in `/temp` dir then remove it after the test is done
@MuhamedMagdi MuhamedMagdi requested a review from Mte90 July 17, 2024 09:11
@Mte90
Copy link
Member

Mte90 commented Jul 17, 2024

So looking at the ticket maybe we should check for permission? @Ph0enixKM

b1ek
b1ek reviewed Jul 17, 2024
View reviewed changes
@b1ek
Copy link
Member

b1ek commented Jul 17, 2024

doesn't work:

image

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

as per the comment above

Mte90
Mte90 reviewed Jul 17, 2024
View reviewed changes
@MuhamedMagdi
Copy link
Contributor Author

doesn't work:

image

image
I think there is a problem with how we translate built in functions as seen above calling unsafe <builtin> gives an error

image
while wrapping it in unsafe block doesn't

@MuhamedMagdi MuhamedMagdi mentioned this pull request Jul 17, 2024
Open
@MuhamedMagdi
fix: add mv to the know keyword list
Loading
Loading status checks…
4710daa
@MuhamedMagdi
Copy link
Contributor Author

Created an issue with this bug in #317

@Mte90 Mte90 requested review from Ph0enixKM and b1ek July 23, 2024 08:55
Ph0enixKM
Ph0enixKM approved these changes Jul 25, 2024
View reviewed changes
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.

Perfectly well-written builtin component. Good job @MuhamedMagdi 👏 Thanks for contribution

Ph0enixKM
Ph0enixKM requested changes Jul 25, 2024
View reviewed changes
Copy link
Member

@Ph0enixKM Ph0enixKM left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry. Just 2 more small little 🤏 things and we're merging this

@MuhamedMagdi
fix: use shorter failed syntax

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
be52966
@MuhamedMagdi
fix: swap back silent values

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
e3b7a5c
@MuhamedMagdi MuhamedMagdi requested a review from Ph0enixKM July 27, 2024 10:24
Ph0enixKM
Ph0enixKM approved these changes Jul 28, 2024
View reviewed changes
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.

LGTM

@MuhamedMagdi
fix: return syntax result
Loading
Loading status checks…
349656e
@Mte90 Mte90 merged commit e0b41d8 into amber-lang:master Jul 29, 2024
1 check passed
@MuhamedMagdi MuhamedMagdi deleted the mv-builtin branch July 29, 2024 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mte90 Mte90

@karpfediem karpfediem

@Ph0enixKM Ph0enixKM

@b1ek b1ek

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@MuhamedMagdi @Mte90 @b1ek @karpfediem @Ph0enixKM