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: improve error messages #347

Merged
merged 13 commits into from
May 30, 2024
Merged

feat: improve error messages #347

merged 13 commits into from
May 30, 2024

Conversation

ComradeVanti
Copy link
Collaborator

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.

Copy link
Member

@favoyang favoyang left a 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?

@ComradeVanti
Copy link
Collaborator Author

@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 cli/index we just ran the commands and depending on if the result was ok or error we exited with 0 or 1. So the results were not really used at runtime at all.

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 Promises instead of AsyncResult. This is also because they have a more imperative and less value-result oriented task.

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.

@favoyang
Copy link
Member

@ComradeVanti thank you for the detailed explanation. LGTM.

@ComradeVanti ComradeVanti merged commit 069970e into master May 30, 2024
6 checks passed
@ComradeVanti ComradeVanti deleted the improve-error-messages branch May 30, 2024 06:59
github-actions bot pushed a commit that referenced this pull request May 30, 2024
# [2.2.0](2.1.1...2.2.0) (2024-05-30)

### Features

* improve error messages ([#347](#347)) ([069970e](069970e))
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.

None yet

2 participants