Add OpenRewrite changes#15665
Conversation
Review Summary by QodoApply OpenRewrite refactoring rules for code quality improvements
WalkthroughsDescription• Applied OpenRewrite refactoring rules across the codebase to improve code quality and consistency • **Removed unnecessary else blocks**: Simplified control flow in 200+ files by eliminating redundant else clauses after early returns and moving statements to top level • **Replaced trim().isEmpty() with isBlank()**: Updated 40+ files to use the more concise and semantically correct String.isBlank() method • **Used Predicate.not for cleaner filtering**: Added static imports and replaced lambda expressions like key -> !key.isEmpty() with not(String::isEmpty) in 15+ files • **Converted to switch expressions**: Modernized code by converting imperative switch statements to switch expressions in select methods • **Improved test annotations**: Changed @CsvSource to @ValueSource for parameterized tests with single values in 10+ test files • **Fixed logic error**: Corrected ETA calculation in ProgressCounter.java by changing <= to < in duration comparison • **Used compound assignment operators**: Replaced verbose assignments like `variable = variable + value with variable += value` in multiple files • **Updated logging**: Converted string concatenation in logger calls to parameterized format strings for better performance • **Modernized collection API**: Changed list.get(0) to list.getFirst() for better code clarity • **Improved formatting**: Fixed indentation and spacing in method chaining and stream operations across multiple files Diagramflowchart LR
A["Codebase<br/>with style issues"] -->|Remove else blocks| B["Simplified<br/>control flow"]
A -->|Replace trim().isEmpty| C["Use isBlank<br/>method"]
A -->|Replace lambdas| D["Use Predicate.not<br/>static import"]
A -->|Update tests| E["Use ValueSource<br/>annotation"]
A -->|Fix logic| F["Correct ETA<br/>calculation"]
B --> G["Improved code<br/>quality"]
C --> G
D --> G
E --> G
F --> G
File Changes1. jablib/src/main/java/org/jabref/logic/citationkeypattern/BracketedPattern.java
|
Code Review by Qodo
1. getLookupResult().get() after isEmpty()
|
|
Does IntelliJ start fighting with OpenRewrite again? --- a/jablib/src/test/java/org/jabref/logic/openoffice/style/OOBibStyleTestHelper.java
+++ b/jablib/src/test/java/org/jabref/logic/openoffice/style/OOBibStyleTestHelper.java
@@ -101,11 +101,11 @@ class OOBibStyleTestHelper {
}
List<CitationMarkerNumericEntry> input =
num.stream()
- .map(n ->
- new CitationMarkerNumericEntryImpl("key" + n,
- n,
- Optional.empty()))
- .collect(Collectors.toList());
+ .map(n ->
+ new CitationMarkerNumericEntryImpl("key" + n,
+ n,
+ Optional.empty()))
+ .collect(Collectors.toList());
return style.getNumCitationMarker2(input, minGroupingCount).toString();
} |
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@timtebeek was so nice to investigate parsing failures - openrewrite/rewrite#7554 (comment) - thank you, Tim! |
I added a reformatting step after OpenRewriteCall. Should mitigate. |
|
Ufff. I have once fixed tests and check style after open rewrite. But I haven’t imagine it’s that much work 😅 |
| } else if (OS.WINDOWS) { | ||
| file = Path.of(address); | ||
| } else { | ||
| if (OS.WINDOWS) { | ||
| file = Path.of(address); | ||
| } else { | ||
| file = Path.of(address.replace("~", System.getProperty("user.home"))); | ||
| } | ||
| file = Path.of(address.replace("~", System.getProperty("user.home"))); |
| } else // When currentUserMessageScroll is set to NEW_NON_EXISTENT_MESSAGE, then we should: | ||
| // 1) either clear the prompt, if user scrolls down the most recent history entry. | ||
| // 2) do nothing, if user starts to edit the history entry. | ||
| // We distinguish these two cases by checking showingHistoryMessage, which is true for -1 message, and false for others. | ||
| if (showingHistoryMessage.get()) { | ||
| userPromptTextArea.setText(""); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Theoretically correct, but stylistically a wtf.
There was a problem hiding this comment.
This is one edge-case. Quoting #15665 (comment)
Please open an issue @InAnYan at https://github.com/openrewrite/rewrite (or maybe https://github.com/openrewrite/rewrite-static-analysis)
| } else { | ||
| // There was no specific editor found | ||
| } | ||
| // There was no specific editor found |
There was a problem hiding this comment.
Comment looks displaced now. Needs more flesh
| if (parentNode == null) { | ||
| return null; | ||
| } else { | ||
| return parentNode.getGroup(); | ||
| } | ||
| return parentNode.getGroup(); |
There was a problem hiding this comment.
Early return pattern is taken too far here. Was better readable and understandable before. Maybe we have to reconfigure the recipies.
| if (sidePane.getChildren().isEmpty()) { | ||
| if (horizontalDividerSubscription != null) { | ||
| horizontalDividerSubscription.unsubscribe(); | ||
| } | ||
| horizontalSplit.getItems().remove(sidePane); | ||
| } else { | ||
| if (!horizontalSplit.getItems().contains(sidePane)) { | ||
| horizontalSplit.setVisible(false); | ||
| horizontalSplit.getItems().addFirst(sidePane); | ||
| Platform.runLater(() -> { | ||
| updateHorizontalDividerPosition(); | ||
| horizontalSplit.setVisible(true); | ||
| }); | ||
| } | ||
| } else if (!horizontalSplit.getItems().contains(sidePane)) { | ||
| horizontalSplit.setVisible(false); | ||
| horizontalSplit.getItems().addFirst(sidePane); | ||
| Platform.runLater(() -> { | ||
| updateHorizontalDividerPosition(); | ||
| horizontalSplit.setVisible(true); | ||
| }); |
There was a problem hiding this comment.
These are two very different categories here. else if suggest they are on the same level. This is going too far.
| } else { | ||
| return Optional.of(suffix); // return the first one we found, anyway. | ||
| } | ||
| } else { | ||
| // Check if there are path separators in the suffix - if so, it is definitely | ||
| return Optional.of(suffix); // return the first one we found, anyway. | ||
| } else // Check if there are path separators in the suffix - if so, it is definitely | ||
| // not a proper suffix, so we should give up: | ||
| if (link.substring(index + 1).indexOf('/') >= 1) { | ||
| return Optional.empty(); | ||
| } else { | ||
| return Optional.of(link.substring(index + 1)); |
| if (StringUtil.isBlank(linkedFile.getDescription())) { | ||
| return ControlHelper.truncateString(linkedFile.getLink(), -1, "...", | ||
| ControlHelper.EllipsisPosition.CENTER); | ||
| } else { | ||
| return ControlHelper.truncateString(linkedFile.getDescription(), -1, "...", | ||
| ControlHelper.EllipsisPosition.CENTER) + " (" + | ||
| ControlHelper.truncateString(linkedFile.getLink(), -1, "...", | ||
| ControlHelper.EllipsisPosition.CENTER) + ")"; | ||
| } | ||
| return ControlHelper.truncateString(linkedFile.getDescription(), -1, "...", | ||
| ControlHelper.EllipsisPosition.CENTER) + " (" + | ||
| ControlHelper.truncateString(linkedFile.getLink(), -1, "...", | ||
| ControlHelper.EllipsisPosition.CENTER) + ")"; | ||
| } |
There was a problem hiding this comment.
Should represent some switch like behaviour. Now looks like early return pattern. Going too far.
| if (object == null) { | ||
| return null; | ||
| } else { | ||
| if (databaseMode == BibDatabaseMode.BIBLATEX) { | ||
| return String.valueOf(object.getNumber()); | ||
| } else { | ||
| return object.getJabRefFormat(); | ||
| } | ||
| } else if (databaseMode == BibDatabaseMode.BIBLATEX) { | ||
| return String.valueOf(object.getNumber()); | ||
| } | ||
| return object.getJabRefFormat(); |
| if (StringUtil.isNotBlank(string)) { | ||
| return Langid.parse(string).orElse(null); | ||
| } else { | ||
| return null; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Not early return pattern. Going to far.
| if ("text/html".equalsIgnoreCase(mimeType)) { | ||
| return Optional.of(HTML_FALLBACK_TYPE); | ||
| } else { | ||
| return Optional.empty(); | ||
| } | ||
| return Optional.empty(); |
There was a problem hiding this comment.
Not early return pattern. Going to far.
| if (Files.isDirectory(pathname)) { | ||
| return true; | ||
| } else { | ||
| return fileFilter.accept(pathname) && !lookup.lookupDatabase(pathname) && !lookup.getPathOfDatabase().equals(pathname); | ||
| } | ||
| return fileFilter.accept(pathname) && !lookup.lookupDatabase(pathname) && !lookup.getPathOfDatabase().equals(pathname); |
There was a problem hiding this comment.
Not early return pattern. Going to far.
| return Stream.of(linkedFile); | ||
| } | ||
| return Stream.of(linkedFile); | ||
| }).toList(); |
There was a problem hiding this comment.
Not early return pattern. Going to far.
| if (so.getOrderType() == SaveOrder.OrderType.TABLE) { | ||
| // We need to "flatten out" SaveOrder.OrderType.TABLE as BibWriter does not have access to preferences | ||
| List<TableColumn<BibEntryTableViewModel, ?>> sortOrder = libraryTab.getMainTable().getSortOrder(); | ||
| return new SelfContainedSaveOrder( | ||
| SaveOrder.OrderType.SPECIFIED, | ||
| sortOrder.stream() | ||
| .filter(col -> col instanceof MainTableColumn<?>) | ||
| .map(column -> ((MainTableColumn<?>) column).getModel()) | ||
| .flatMap(model -> model.getSortCriteria().stream()) | ||
| .toList()); | ||
| } else { | ||
| return SelfContainedSaveOrder.of(so); | ||
| } | ||
| return SelfContainedSaveOrder.of(so); |
There was a problem hiding this comment.
Not early return pattern. Going to far.
| if (parserResult.hasWarnings()) { | ||
| LOGGER.warn("Could not store entry: {}", parserResult.warnings()); | ||
| String errors = parserResult.getErrorMessage(); | ||
| dialogService.showErrorDialogAndWait(errors); | ||
| validationMessage.setValue(ValidationMessage.error(Localization.lang("Failed to parse Bib(La)TeX: %0", errors))); | ||
| return; | ||
| } else { | ||
| LOGGER.warn("No entries found."); | ||
| String errors = Localization.lang("No entries available"); | ||
| dialogService.showErrorDialogAndWait(errors); | ||
| validationMessage.setValue(ValidationMessage.error(Localization.lang("Failed to parse Bib(La)TeX: %0", errors))); | ||
| return; | ||
| } | ||
| LOGGER.warn("No entries found."); | ||
| String errors = Localization.lang("No entries available"); | ||
| dialogService.showErrorDialogAndWait(errors); | ||
| validationMessage.setValue(ValidationMessage.error(Localization.lang("Failed to parse Bib(La)TeX: %0", errors))); | ||
| return; |
There was a problem hiding this comment.
Not early return pattern. Going to far.
There was a problem hiding this comment.
Stopped reviewing. The rewrite recipies are too comprehensive.
Early return pattern is applicated too harshly, often obfuscating and disfiguring deliberately chosen parallelisms in code style.
Merging this in the current state would vastly reduce code readability and code quality.
Pull request was converted to draft
|
Okay, then probably better to turn that recipe off and then view the PR. Maybe I’ll do it today’s evening on/after the call |
|
Thanks. I'm thinking about how to distinguish real early return cases from switch like if statements, but I'm not sure if this is possible at all to configure in rewrite. |
|
Not to be misunderstood, reactivation and implementation of rewrite changes are necessary, yet the early return rule seems to strong. |
|
Perhaps it helps to convert the switch-like |
|
In theory yes, depends maybe on the complexity of the constraints. Please open an issue. |
Thank you for the hint. This seems to be a manual checking process. The I really wonder how this came in - but maybe, because our check worfklow did not work properly. |
| } else { | ||
| return ManageKeywordsDisplayType.CONTAINED_IN_ALL_ENTRIES; | ||
| } | ||
| return ManageKeywordsDisplayType.CONTAINED_IN_ALL_ENTRIES; |
There was a problem hiding this comment.
I agree that this is strange to read -- maybe, because the fail-fast length is equal to the other length?
Related issues and pull requests
Closes https://github.com/JabRef/jabref-issue-melting-pot/issues/1289
PR Description
As OpenRewrite check wasn't working properly, a lot of changes was accumulated. I run OpenRewrite on the current main and this is the PR.
Steps to test
rewriteDryRun.As a result, they all should be green.
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)