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 logging messages #4208

Merged
merged 2 commits into from
Jul 8, 2024
Merged

Conversation

logresearch
Copy link
Contributor

@logresearch logresearch commented Jul 2, 2024

Description

This pull request addresses the issues with the logging statements in the fromContentUri method. The changes improve the clarity and context of the log messages when navigating to file paths.

The original code is confusing to understand.

File pathFile = new File(uri.getPath().substring(FILE_PROVIDER_PREFIX.length() + 1));
    if (!pathFile.exists()) {
      LOG.warn("failed to navigate to path {}", pathFile.getPath());
      pathFile = new File(uri.getPath());
      LOG.warn("trying to navigate to path {}", pathFile.getPath());
    }
    return pathFile;

Changes:

  1. Initial Path Navigation Failure Log:

    • Original:
      LOG.warn("failed to navigate to path {}", pathFile.getPath());
    • Improved:
      LOG.warn("Failed to navigate to the initial path: {}", pathFile.getPath());
    • Reason: Provides more context by indicating that this is the initial path navigation attempt.
  2. Fallback Path Navigation Log:

    • Original:
      LOG.warn("trying to navigate to path {}", pathFile.getPath());
    • Improved:
      LOG.warn("Attempting to navigate to the fallback path: {}", pathFile.getPath());
    • Reason: Clarifies that this is a second attempt to navigate to a fallback path.

These improvements will help in better understanding and diagnosing issues related to file path navigation failures.

Automatic tests

  • Added test cases

Manual tests

  • Done

Build tasks success

Successfully running following tasks on local:

  • ./gradlew assembledebug
  • ./gradlew spotlessCheck

@VishnuSanal
Copy link
Member

thanks for your interest in contributing to amaze. but, I am afraid I don't think this is a mandatory change to be made.

@VishnuSanal VishnuSanal closed this Jul 2, 2024
@logresearch
Copy link
Contributor Author

thanks for your interest in contributing to amaze. but, I am afraid I don't think this is a mandatory change to be made.

Hi @VishnuSanal ,

Thanks for your quick reply.
We are doing research on automatically maintain the quality of logging statement.
We would be grateful if the developers for popular repositories think it's useful.
We are looking forward for your opinion for automatically LS quality maintenance!

@VishalNehra
Copy link
Member

Looks fine to me. Any code improvements are welcome whether it's for readability or actually fixes / issues.

@VishalNehra VishalNehra reopened this Jul 2, 2024
@logresearch
Copy link
Contributor Author

logresearch commented Jul 3, 2024

Hi @VishalNehra,

Thanks for your positive reply!
Really hope my contribution can make AmazeFileManagermore perfect, though the contribution is incremental.
Based on the feedback, I further reviewed some source code and logging messages and found the following inaccurate expression.

https://github.com/TeamAmaze/AmazeFileManager/blob/release/4.0/app/src/main/java/com/amaze/filemanager/asynchronous/loaders/AppListLoader.java#L87
LOG.warn("faield to find android package name while loading apps list", e);
The faield is a typo, which should be failed

Also the same in https://github.com/TeamAmaze/AmazeFileManager/blob/release/4.0/app/src/main/java/com/amaze/filemanager/asynchronous/loaders/AppListLoader.java#L102
faield -> failed

We update them in this PR together.

@VishalNehra VishalNehra merged commit cb2b2e3 into TeamAmaze:release/4.0 Jul 8, 2024
1 check passed
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.

3 participants