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

Use stream to look for error message prefixes #331

Merged
merged 2 commits into from
Mar 15, 2022
Merged

Use stream to look for error message prefixes #331

merged 2 commits into from
Mar 15, 2022

Conversation

avimanyum
Copy link
Contributor

This PR creates an Array of potential error messages and streams through them to check for the existence of one. This way we will not have to modify the conditional check even if there are more error prefixes we add in the future.

@github-actions github-actions bot added the tests Improvements / maintenance to tests label Mar 14, 2022
@@ -21,6 +22,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Arrays;
Copy link

Choose a reason for hiding this comment

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

Wrong lexicographical order for 'java.util.Arrays' import. Should be before 'org.slf4j.LoggerFactory'.

@codecov
Copy link

codecov bot commented Mar 14, 2022

Codecov Report

Merging #331 (82fd511) into master (e7fd34b) will increase coverage by 0.03%.
The diff coverage is 85.71%.

❗ Current head 82fd511 differs from pull request most recent head e348ed3. Consider uploading reports for the commit e348ed3 to get more accurate results

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #331      +/-   ##
============================================
+ Coverage     81.55%   81.59%   +0.03%     
  Complexity      265      265              
============================================
  Files            19       19              
  Lines           938      940       +2     
  Branches        127      126       -1     
============================================
+ Hits            765      767       +2     
  Misses          142      142              
  Partials         31       31              
Impacted Files Coverage Δ
...sforce/dockerfileimageupdate/utils/GitHubUtil.java 73.33% <85.71%> (+0.51%) ⬆️

Comment on lines +121 to 123
String finalError = error;
List<String> errorPrefixes = Arrays.asList(PR_INVALID_CODE, "No commits between");
if (error.startsWith("A pull request already exists")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why you need finalError instead of just using error but you should probably use either one or the other in both if statements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I tried using the variable error I got the following warning on IDE
Variable used in lambda expression should be final or effectively final

// This case usually happens when the PR head has invalid data. Deleting the forked repo resolves it.
error = PR_INVALID_CODE;
String error = PR_INVALID_CODE;
JsonObject errorMessage = errorJson.get(0).getAsJsonObject();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there ever more than 1 error in this array? Any reason not to foreach on them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about that. This piece of code has been there for a while.

@afalko afalko merged commit 75474c7 into salesforce:master Mar 15, 2022
@justinharringa justinharringa added bug Bug fixes and removed tests Improvements / maintenance to tests labels Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants