-
-
Notifications
You must be signed in to change notification settings - Fork 13
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: improve error messages #347
Conversation
Allow using expect matcher for prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
It appears that you've abandoned the use of ts-results at the cmd-xxx level and have returned to using a simple return code and inline logging. What's the motivation for this move?
@favoyang you are right. In order to explain let me first remind you why I introduced results on the cmd layer in the first place. This was done in order to better test the different reasons why the cmd might fail. instead of just checking everywhere that we got a 1 result code, we could, through the result object determine what actually went wrong. So this improved testability. At runtime it did not really change anything. Inside Since then a few things have changed. For this new change the most important part is the different responsibilities of cmd and service functions. Cmd functions now should only contain CLI related code. You could think of this as the applications UI layer. The services contain all the business logic, independent of frontend. So we should also be able to use these inside a theoretical openupm GUI application. This separation is an ongoing process. For example the add and remove commands still do a lot of business logic. But the other cmds are now completely empty of non-cli business logic. With this introduction of the service layer and the move of the business logic there, the tests have also shifted. Now the different results are tested inside the service layer. The cmd layer is now only responsible for presenting these results, ie. turning them into strings to be printed and a numerical exit code. Because of this we can simplify the cmd functions by returning simple numeric codes instead of the, sometimes cumbersome, result objects, since we no longer get any benefit from them. Also notice that the cmd functions are now pretty much the only functions in the projects which return pure I hope this clears it up. I also want to describe this design thinking approach I have brought to the project inside developer documentation at some point. |
@ComradeVanti thank you for the detailed explanation. LGTM. |
# [2.2.0](2.1.1...2.2.0) (2024-05-30) ### Features * improve error messages ([#347](#347)) ([069970e](069970e))
This PR adds a bunch of new error messages and fix suggestions for the various things that can go wrong when using openupm.
It also makes some improvements to error types.
Also also cmd functions now simply return a numeric result code that is used for the process exit code. They should log all possible errors before returning.