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 BibDatabase Migrations #3733

Merged
merged 5 commits into from
Feb 20, 2018
Merged

Refactor BibDatabase Migrations #3733

merged 5 commits into from
Feb 20, 2018

Conversation

LinusDietz
Copy link
Member

This is the followup for #3658 (comment).

I have moved the functionality from org.jabref.migrations to org.jabref.logic.importer.migrations.

The code is called from the UI now in org.jabref.gui.importer.actions.OpenDatabaseAction.performPostOpenActions(), where also some other similar actions are called.

Since the build is broken at the moment due to external resources, I'll have to re-check later if everything is actually fine here.
The general structure should be ready to review anyways.

@LinusDietz LinusDietz added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers import labels Feb 17, 2018
@stefan-kolb
Copy link
Member

Why logic.importer.migrations? Sure it would not be in a wrong place, but we put it in a top-level package on purpose as this should be immediately evident that this is caused by JabRef design decisions and not hidden inside logic.importer where standard formats should be put. Not sure what others think.

@Siedlerchr
Copy link
Member

Agree with @stefan-kolb What has a migration to do with import?
Top level logic.migrations should be the way to got

@lenhard
Copy link
Member

lenhard commented Feb 17, 2018

+1 for logic.migrations

@@ -1,4 +1,4 @@
package org.jabref.migrations;
package org.jabref.logic.importer.migrations;
Copy link
Member

Choose a reason for hiding this comment

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

This class has to many UI-dependencies to be in logic.

@Siedlerchr
Copy link
Member

The failing test is probably due to caching on travis and a result from #3713.
I cleared the cache in travis for master and this build. Hope that's enough.,
If not, delete it again: https://travis-ci.org/JabRef/jabref/caches

@koppor koppor mentioned this pull request Feb 18, 2018
8 tasks
import org.jabref.logic.importer.ParserResult;
import org.jabref.logic.migrations.MergeReviewIntoCommentMigration;

public class MergeReviewIntoComment implements GUIPostOpenAction {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename that with the action suffix so that it is better distingusishable from the other migration

import org.jabref.logic.l10n.Localization;
import org.jabref.model.entry.BibEntry;

public class MergeReviewIntoCommentUIManager {
public class MergeReviewIntoCommentConfirmation {
Copy link
Member

Choose a reason for hiding this comment

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

I would add the Dialog suffix to better see that above

MergeReviewIntoCommentMigration migration = new MergeReviewIntoCommentMigration();

migration.performMigration(parserResult);
if (new MergeReviewIntoCommentConfirmation(basePanel).askUserForMerge(MergeReviewIntoCommentMigration.collectConflicts(parserResult))) {
Copy link
Member

Choose a reason for hiding this comment

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

I would split that into assignment and declaration. It's unforunately not so easy distinguishable what what ist, because all is named very similar

Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

If you make the class names a bit more distinguishable from each other, I would be happy.
Rest is okay 👍

Copy link
Member

@lenhard lenhard left a comment

Choose a reason for hiding this comment

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

Code looks good and I'll merge. At some point, we can think about removing the FileLinksUpgradeWarning migration completely.

v2.3 is so long back and by removing this we could speed up the startup a little.

@lenhard lenhard merged commit e9e7bcc into master Feb 20, 2018
@lenhard lenhard deleted the refactor-migrations branch February 20, 2018 13:57
@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants