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

fix: Revert capitalization in error_media_upload_image_or_video #813

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mileskrell
Copy link
Contributor

This appears to have been changed by mistake.

@mileskrell mileskrell changed the title (fix): Revert capitalization in error_media_upload_image_or_video fix: Revert capitalization in error_media_upload_image_or_video Jul 10, 2024
@nikclayton
Copy link
Contributor

It's a deliberate change, although one that's probably going to take some time to complete.

Refactoring error management and display

I'm doing a background refactor of how errors are handled and shown to the user. As a general principle I'm trying for the errors to useful for the user so they have some idea of what's wrong and how they might fix it, and containing enough information so that a developer can root cause the problem.

So errors are "chained". Suppose you have an architecture with layers like:

UI -> ViewModel -> Repository -> Storage-or-Network

(possibly with usecases between the ViewModel and Repository).

The user performs an action in the UI layer, which triggers a chain of actions going from left to right. An error might occur at any of those layers. When it does the error is either handled at that layer (e.g., retry the operation), or is returned to the previous layer (right-to-left) where the layer can either handle the error or return it.

Each layer has additional context[1] about what the user was trying to do. So as the error moves from right to left the layer "wraps" the original error in its own error which can carry that additional context.

If unhandled the error eventually makes its way back to the UI layer for handling. At this layer handling the error typically means updating the UI in some fashion and displaying the error, possibly with a "Retry" button.

To display the error the error message must be constructed. To do that each error has an optional string resource and a fmt method that interpolates any of its own context in to the string resource.

The error message is constructed error by error, in to the final error message. By convention:

  1. The UI error message should start with a capital letter
  2. The other error messages are interpolated in to the final error message with a language appropriate separate; for English that's :
  3. Because the lower layer error messages are not the start of a sentence they shouldn't start with a capital letter (unless it's an acronym or initialism).

So you end up with an error message that looks like this:

Error message from ViewModel: error message from repository: error message from storage-or-network

E.g (hypothetically):

Following Miles Krell failed: your server returned "This action is not allowed": HTTP 403

All of this machinery is in the PachliError interface and classes that implement it. If you look at ApiError you'll see where the low level API errors are handled.

SuggestionsRepository shows how a repository can provide its own errors but doesn't need to provide any additional context; all three errors from that repository wrap the ApiError. It's important that they wrap it, and not pass it through as is, because if they did that SuggestionsViewModel would need to know about the ApiError class, breaking encapsulation. With this approach each layer only needs direct knowledge of the errors that can be returned from the layer it's talking to.

This is an ongoing, lengthy refactoring which I'm doing in between other bits of work (I've almost finished refactoring the filters code to do this). There's lots of code that doesn't do this yet, and as I'm doing the work I'm adjusting the idioms slightly as I go, as I discover slightly different uses along the way.

Move exception handling to the right

The other reason I'm doing this is to push exception handling as far to the right in that architecture diagram as possible. Storage and network layers can throw exceptions, there's not much we can do about that (IOException, HttpException, etc). But it's the responsibility of the repository layer to ensure that any exceptions are caught and wrapped in something that implements PachliError so they can be handled appropriately and passed back to the previous layer.

Since Kotlin doesn't have checked exceptions I'm using a Result type to do this. The built-in Kotlin Result type assumes that all errors are subclasses of Throwable which is not always correct, so I'm using https://github.com/michaelbull/kotlin-result/tree/1.1.21.

Pair?

If you'd like to work on any of this let me know. I've got my eye on the trending tags code as the next thing to refactor, that might be a good opportunity to do some remote pair-programming.


[1] In the English sense of the word, not the Android Context sense.

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