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

feat: implement uname and which as builtin functions #124

Closed
wants to merge 3 commits into from
Closed

Conversation

wolfv
Copy link
Member

@wolfv wolfv commented Sep 14, 2024

Seems to work quite well. Let me know what you think @certik / @prsabahrami :)

@certik
Copy link
Collaborator

certik commented Sep 14, 2024

I think we should remove the codecov/patch, since it always fails. I like the CI to be green. :)

Copy link
Collaborator

@certik certik left a comment

Choose a reason for hiding this comment

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

I tested on Windows, both which and uname works:

~/repos/shell(uname-which)$ which git
C:\Program Files\Git\cmd\git.exe
~/repos/shell(uname-which)$ uname -a
Windows_NT LAPTOP-XYE11GAT 10.0 22631 x86_64 MS/Windows (Windows 11)

@wolfv
Copy link
Member Author

wolfv commented Sep 14, 2024

Should we merge then?! :)

@certik
Copy link
Collaborator

certik commented Sep 15, 2024

Go for it!

@wolfv
Copy link
Member Author

wolfv commented Sep 15, 2024

I think I should remove a few unwrap's to make things nice.

@prsabahrami
Copy link
Collaborator

I think it's great. Though, as you mentioned, I think it's better to replace the unwraps with proper error handling. =)

@prsabahrami
Copy link
Collaborator

Also, @wolfv do you think you could add some tests? It would be great to start implementing tests for the newer features so they won't get stacked up altogether.

@prsabahrami prsabahrami mentioned this pull request Sep 16, 2024
@wolfv
Copy link
Member Author

wolfv commented Sep 17, 2024

Already in thanks to @prsabahrami in #135

@wolfv wolfv closed this Sep 17, 2024
@wolfv wolfv deleted the uname-which branch September 21, 2024 07:33
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.

3 participants