-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
[WIP] Fix issue no warning message #11904
[WIP] Fix issue no warning message #11904
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code currently does not meet JabRef's code guidelines.
We use OpenRewrite to ensure "modern" Java coding practices.
The issues found can be automatically fixed.
Please execute the gradle task rewriteRun
, check the results, commit, and push.
You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution can be made more simpler: Use org.jabref.logic.util.NotificationService
. Pass it down to the CleanupWorker.
} catch (FileSystemException exception) { | ||
fileSystemExceptions.add(exception); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not adding all IOException
? They are logged below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this change based on the previous PR (#11484). I also think that it's better to add all of IOException.
Thanks for the input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think, we overlooked that thing in the PR (because of other comments 😅).
Side thing: Don't forget to run the OpenRewrite task and check checkstyle 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code currently does not meet JabRef's code guidelines.
We use OpenRewrite to ensure "modern" Java coding practices.
The issues found can be automatically fixed.
Please execute the gradle task rewriteRun
, check the results, commit, and push.
You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".
@juliusalberto Does the close mean that you also want to be assigned at #10121? |
@koppor I think I'd create a new PR that is not a draft PR (I rewrote it from the main branch because reverting the commits is too much hassle). I will reference this PR in my new one |
Without a new branch, you can do follow steps to start from scratch
But nevermind. Maybe next time. |
Superseded by #11987 |
Ref: #10121
This is a draft PR for the issue 10121. I made these changes based on the previous PR that was not merged - see #11484
I just want to have a sanity check on my approach to the issue. From what I read in the issue, the challenges are:
I decided to tackle this issue by:
MoveFilesCleanup
:getFileSystemExceptions()
method to retrieve these exceptions.CleanupWorker
:MoveFilesCleanup
and if so, collects its exceptions.getFileSystemExceptions()
method to retrieve all collected exceptions.CleanupAction
:CleanupWorker
.DialogService
.Currently this is what is showing in my screen:
![image](https://private-user-images.githubusercontent.com/57690582/375357059-867eaa40-0b87-4705-93e7-1565d30ecc6d.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk1OTg2NDMsIm5iZiI6MTczOTU5ODM0MywicGF0aCI6Ii81NzY5MDU4Mi8zNzUzNTcwNTktODY3ZWFhNDAtMGI4Ny00NzA1LTkzZTctMTU2NWQzMGVjYzZkLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMTUlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjE1VDA1NDU0M1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTM5M2U0YTg0OTVmYTZjOTIxYjQ4Njk5MWUyNWNjOWVhZjRiMThmOWM4Nzg0MzA4MTc2MGZlNTVhMTEyMTIwZjEmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.dcK4xrjGoaTn3pKIvDlWziuvsu64OOJtGWPYMtCDh00)
Question:
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if applicable)