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

refactor(mobile): move error details to separate DB column #6898

Merged
merged 8 commits into from Feb 24, 2024

Conversation

rovo89
Copy link
Contributor

@rovo89 rovo89 commented Feb 3, 2024

As discussed in #6866, logRecord.error previously wasn't stored in the DB. Instead, a string copy of it was merged into the message in most places. Separate the two, so the log overview shows a brief description of the error while the details (usually an exception, but could be any string) can be found in the details page.

I adjusted many places to split the log message into its headline and the details, but for sure there will be some that I've missed. It shouldn't be a problem to adjust those later. There might also be places where adding further details as string could be helpful, like a few statistics when the upload has finished.

@rovo89
Copy link
Contributor Author

rovo89 commented Feb 3, 2024

@shenlong-tanwen: PTAL

Copy link
Member

@shenlong-tanwen shenlong-tanwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the PR!

Copy link
Member

@shenlong-tanwen shenlong-tanwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

mobile/lib/shared/views/app_log_detail_page.dart Outdated Show resolved Hide resolved
@rovo89
Copy link
Contributor Author

rovo89 commented Feb 23, 2024

That's funny... first it complains that

          final failedResponse =
              imageResponse.statusCode != 200 ? imageResponse : motionReponse;
          _log.severe("Motion asset download failed", failedResponse.toLoggerString());
          return false;

is bad and formats it to

          final failedResponse =
              imageResponse.statusCode != 200 ? imageResponse : motionReponse;
          _log.severe(
              "Motion asset download failed", failedResponse.toLoggerString());
          return false;

(which is less readable from my point of view).
But somehow the linter isn't friends with the formatter and wants to have a final comma:

          final failedResponse =
              imageResponse.statusCode != 200 ? imageResponse : motionReponse;
          _log.severe(
              "Motion asset download failed", failedResponse.toLoggerString(),);
          return false;

The end? No, the formatter wants some changes again:

          final failedResponse =
              imageResponse.statusCode != 200 ? imageResponse : motionReponse;
          _log.severe(
            "Motion asset download failed",
            failedResponse.toLoggerString(),
          );
          return false;

Looks better at least, but it pushed a simply log message from one to four lines and made it seem more important than it needs to be. Similar for the line above that. I thought the formatter is supposed to improve readability but, the 80 character limit achieves the opposite in my eyes (literally 😁).

OK. Rant over, Dart seems to be happy now, so hopefully ready to merge. 😉

@alextran1502 alextran1502 merged commit bc39790 into immich-app:main Feb 24, 2024
24 checks passed
@rovo89 rovo89 deleted the log_error_details branch February 24, 2024 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants