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(cli): exit codes #52

Merged
merged 1 commit into from
Feb 25, 2023
Merged

Conversation

thepassle
Copy link
Contributor

Alright, I took a stab at implementing #50

Some things of interest:

  • I refactored the lint and lint_path methods a little bit to access the diagnostics in main.rs
  • Clippy complains about the println!() in main.rs, but I dont know how to make it happy 🙃
  • In lib.rs, I get a compiler warning that the path and diagnostics never get used, even though I assign them in line 35 in Cli, I'm guessing thats because the diagnostic already gets printed in lint_path, maybe I should move the printing of the diagnostics over to main.rs as well? What do you think?

crates/oxc_cli/src/main.rs Outdated Show resolved Hide resolved
@Boshen
Copy link
Member

Boshen commented Feb 25, 2023

I think what's cool about passing data into PathNotFound and LintResult is that we can combine common errors and print them here.

So something like

            Self::PathNotFound { path } => {
                println!("Path {} does not exist.", path.to_string_lossy());
                ExitCode::from(1)
            }
            Self::LintStats {
                run_duration,
                number_of_diagnostics,
            } => {
                if number_of_files == 0 {
                    println!("No files found.");
                    ExitCode::from(0)
                } else {
...

Because once we get multiple commands such as oxc lint / minify / prettier, print and then exit will become super messy. It's a lot cleaner to delegate these kind of common printing logic here.

@Boshen
Copy link
Member

Boshen commented Feb 25, 2023

Clippy complains about the println!() in main.rs, but I dont know how to make it happy 🙃

Let me guess it wants you to do println!("{variable}") instead of println!("{}", variable)?

@Boshen
Copy link
Member

Boshen commented Feb 25, 2023

maybe I should move the printing of the diagnostics over to main.rs as well

I think this sounds good, or you can print them in the report method, since we are going to get other commands that emit diagnostics, which'll end up with the same printing logic.

crates/oxc_cli/src/lib.rs Outdated Show resolved Hide resolved
@thepassle
Copy link
Contributor Author

Because once we get multiple commands such as oxc lint / minify / prettier, print and then exit will become super messy. It's a lot cleaner to delegate these kind of common printing logic here.

That makes sense, let me try to refactor that

Let me guess it wants you to do println!("{variable}") instead of println!("{}", variable)?

TIL, will fix

I think this sounds good, or you can print them in the report method, since we are going to get other commands that emit diagnostics, which'll end up with the same printing logic.

Yep, makes sense

@Boshen
Copy link
Member

Boshen commented Feb 25, 2023

This kind of Rust takes a while, but I think you are getting the hang of it, I hope you are having fun!

@thepassle
Copy link
Contributor Author

This kind of Rust takes a while, but I think you are getting the hang of it, I hope you are having fun!

For sure, this is great practice 🙂

I think I covered all the review feedback, could you take another look and let me know what you think? Thanks!

foo.js Outdated Show resolved Hide resolved
@Boshen
Copy link
Member

Boshen commented Feb 25, 2023

Do you mind squash and rebase your commits? It's almost ready to go!

@Boshen
Copy link
Member

Boshen commented Feb 25, 2023

When you get a "[Merge branch 'main' into feat/exit-codes]" commit, it means you haven't pulled the main branch yet so git start merging your branch.

Can you try:

git checkout main
git pull
git checkout -
git rebase main
git push --force

@thepassle thepassle force-pushed the feat/exit-codes branch 2 times, most recently from 1324019 to 12aeea4 Compare February 25, 2023 17:22
@thepassle
Copy link
Contributor Author

Sorry, I had to update my fork, should be rebased correctly now

chore: remove foo.js

chore: review comments

chore: shut clippy up

chore: ran cargo fmt

chore: unnecessary return
@Boshen
Copy link
Member

Boshen commented Feb 25, 2023

You may want to setup auto format, it's better than what we have in js ;-)

@Boshen Boshen merged commit eaeafa0 into oxc-project:main Feb 25, 2023
@Boshen
Copy link
Member

Boshen commented Feb 25, 2023

Thank you!

@thepassle
Copy link
Contributor Author

Thanks for bearing with me! 🙂 Would love to hack on something else if you have any other good first issues popping up!

@Boshen
Copy link
Member

Boshen commented Feb 25, 2023

Thanks for bearing with me! 🙂 Would love to hack on something else if you have any other good first issues popping up!

Checkout all the issues I created :-)

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