check for parsing errors

[corbanvilla]

Fix for #897

The default error fmt method from clap works great, so just essentially printing that.

Not sure if it's preferred to exit in main.rs or here, but this seemed appropriate to me.

[ghaith]

Fix for #897

The default error fmt method from clap works great, so just essentially printing that.

Not sure if it's preferred to exit in main.rs or here, but this seemed appropriate to me.

Thank you for the contribution!

It is indeed preferred to do this in main.rs, the reason is that the library should still return a compilation result and it now simply exits the program. I think it is better suited for main to decide when to exit.

Furthermore, the error type provides an exit() method. If we use this in main, it will exit with the correct messages and status. This used to be the implementation before we moved to the pipeline. To be honest, I quickly wrote main when we did the move and forgot about it.

https://github.com/PLC-lang/rusty/blob/v0.2.0/src/main.rs

[codecov[bot]]

Codecov Report

Patch coverage: 36.36% and project coverage change: -0.04% :warning:

Comparison is base (0105dc0) 96.00% compared to head (749fadf) 95.96%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #916 +/- ## ========================================== - Coverage 96.00% 95.96% -0.04% ========================================== Files 128 128 Lines 38761 38781 +20 ========================================== + Hits 37212 37218 +6 - Misses 1549 1563 +14 ``` | [Files Changed](https://app.codecov.io/gh/PLC-lang/rusty/pull/916?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PLC-lang) | Coverage Δ | | |---|---|---| | [compiler/plc\_driver/src/main.rs](https://app.codecov.io/gh/PLC-lang/rusty/pull/916?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PLC-lang#diff-Y29tcGlsZXIvcGxjX2RyaXZlci9zcmMvbWFpbi5ycw==) | `11.11% <0.00%> (-3.18%)` | :arrow_down: | | [compiler/plc\_driver/src/lib.rs](https://app.codecov.io/gh/PLC-lang/rusty/pull/916?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PLC-lang#diff-Y29tcGlsZXIvcGxjX2RyaXZlci9zcmMvbGliLnJz) | `87.36% <42.10%> (-5.33%)` | :arrow_down: | ... and [4 files with indirect coverage changes](https://app.codecov.io/gh/PLC-lang/rusty/pull/916/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PLC-lang)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[corbanvilla]

Happy to help out, and thank you for the feedback!

It looks like the exit method is part of the clap Error type, not the Diagnostic one, which makes calling it a little more tricky. I'm thinking of a couple ways we could do that...

• Include the error type as an optional in General error, or include it as a more specific argument parsing error.
• Parse the arguments before passing into the compile function.
• Include the return code as part of general error.

Let me know what you think is best.

[ghaith]

Happy to help out, and thank you for the feedback!

It looks like the exit method is part of the clap Error type, not the Diagnostic one, which makes calling it a little more tricky. I'm thinking of a couple ways we could do that...

* Include the error type as an optional in General error, or include it as a more specific argument parsing error.

* Parse the arguments before passing into the compile function.

* Include the return code as part of general error.

Let me know what you think is best.

I think parsing the parameters before the lib seems like the most logical way but would require a lot of code change in the tests and standard lib. Another way would be to introduce a new Diagnostic type, a Diagnostic::ParameterError(clap::Error) and then implement the From for diagnostics, this way could have something similar to how the main used to be structured. The parameter error type will still do the right thing with exit().

If you go with the option of parsing the error in main before calling compile, please also introduce the method in the lib so we could use it from the standard lib and test code.

[corbanvilla]

It looks like clap::Error doesn't implement clone, so adding a new type to Diagnostic is likely a no-go, since Diagnostic does have clone.

Agreed, I'll work on a function in lib to handle parsing errors.

How about a function to essentially parse the args and proxy the compile. Then maybe the Result error type can be an enum, either Diagnostic or clap::Error... (this still feels a bit clunky, but does minimize changes for tests).

parse_args_and_compile(args: &T) -> Result<()>

Otherwise, I can just do a function which only parses, and just deal with the test changes:

parse_compile_parameters(args: &T) -> Result<CompileParameters,ParameterError> 

[ghaith]

Otherwise, I can just do a function which only parses, and just deal with the test changes:

parse_compile_parameters(args: &T) -> Result<CompileParameters,ParameterError> 

This seems like the best option for now. So the lib has a parse method to get parameters and an entry point method that starts from the parameters. I believe the standard lib build script also needs to reflect this, but you will notice this when compiling. Make sure you compile with --workspace for that

[ghaith]

Hello @corbanvilla I updated your change and moved the exit logic to main, by creating an intermediate error type. This reduces the test changes while keeping the exit logic in main instead of the lib.