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

Add better help texts for Error::ShellProgramNotFound #4082

Merged
merged 13 commits into from
Dec 23, 2024

Conversation

enkerewpo
Copy link
Contributor

add bun, deno, elixir and nodejs installation helper texts and improved per OS instructions

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.

Cool! Thank you! Could you add snapshot tests for the new functionality please 🙏

compiler-core/src/error.rs Outdated Show resolved Hide resolved
compiler-core/src/error.rs Outdated Show resolved Hide resolved
compiler-core/src/error.rs Outdated Show resolved Hide resolved
compiler-core/src/error.rs Outdated Show resolved Hide resolved
@lpil lpil marked this pull request as draft December 16, 2024 11:28
@enkerewpo
Copy link
Contributor Author

enkerewpo commented Dec 16, 2024

hi there, I wrote a new snapshot test in compiler-core/src/error/tests.rs and the snap files were commited in a seperate commit

best regards

@enkerewpo enkerewpo requested a review from lpil December 16, 2024 13:23
@enkerewpo enkerewpo marked this pull request as ready for review December 17, 2024 01:40
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! I've left a few small notes inline and then we're ready to go!

compiler-core/src/error.rs Outdated Show resolved Hide resolved
compiler-core/src/error.rs Outdated Show resolved Hide resolved
compiler-core/src/error.rs Outdated Show resolved Hide resolved
@enkerewpo
Copy link
Contributor Author

enkerewpo commented Dec 17, 2024

Hi there,
I changed them to enums and updated the test codes and snapshots.
Best regards

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! A few small notes inline

compiler-core/src/error.rs Outdated Show resolved Hide resolved
compiler-cli/src/fs.rs Outdated Show resolved Hide resolved
compiler-cli/src/fs.rs Outdated Show resolved Hide resolved
compiler-core/src/error.rs Outdated Show resolved Hide resolved
compiler-core/src/error.rs Outdated Show resolved Hide resolved
compiler-core/src/error.rs Outdated Show resolved Hide resolved
@enkerewpo enkerewpo force-pushed the main branch 3 times, most recently from 6aa4cda to 17eaab0 Compare December 20, 2024 15:27
@enkerewpo
Copy link
Contributor Author

enkerewpo commented Dec 20, 2024

Hi,

I re-constructed the code :)

Now Distro is inside Linux and top level will only need to pass program name and OS struct(via get_os) to construct the Error.

And I split the get_distro to only acquire the os-release ID string and nothing else, then let parse_linux_distribution and parse_os to get the final OS structure.

Best regards

@enkerewpo enkerewpo requested a review from lpil December 20, 2024 16:10
@enkerewpo
Copy link
Contributor Author

enkerewpo commented Dec 21, 2024

I wrote standalone tests for extracting NAME=yyy...ID=xxx... to xxx in compiler-cli/src/fs/tests.rs:extract_distro_id_test()

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.

Fantastic work! Just a couple more tiny tweaks, and if you could add an empty line between the package manager suggestion and the docs link that'd be fab. Ideally the docs link would be the last thing in the message.

@enkerewpo
Copy link
Contributor Author

OK 🦊

@enkerewpo enkerewpo requested a review from lpil December 22, 2024 03:22
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.

Bravo!! Thank you so much!

@lpil lpil enabled auto-merge (rebase) December 23, 2024 10:50
@lpil lpil merged commit 42a20a2 into gleam-lang:main Dec 23, 2024
12 checks passed
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.

2 participants