-
Notifications
You must be signed in to change notification settings - Fork 3
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
41 support more customization #53
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.
Sieht gut aus würd ich sagen 😄 . Das meiste ist entweder Geschmakssache oder Kosmetik
.putInstructions("Some_process_definition_key, Arrays.asList( | ||
//use the prepared way of specifying instructions or implement your own | ||
return new MigrationInstructionsMap() | ||
.putInstructions("Some_process_definition_key, Arrays.asList( |
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.
Schwerer Fehler, hier fehlt ein Anführungszeichen 😮 .
.putInstructions("Some_process_definition_key, Arrays.asList( | ||
//use the prepared way of specifying instructions or implement your own | ||
return new MigrationInstructionsMap() | ||
.putInstructions("Some_process_definition_key, Arrays.asList( | ||
MinorMigrationInstructions.builder() | ||
.sourceMinorVersion(0) | ||
.targetMinorVersion(2) | ||
.majorVersion(1) |
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.
Nur Kosmetik, aber die Reihenfolge major -> minor ist eventuell schöner zu lesen.
*/ | ||
@NoArgsConstructor | ||
public class ProcessInstanceMigratorBuilder { | ||
|
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.
Eine reset Methode wäre bei den ganzen Builder Klassen vielleicht noch hilfreich
* @return a list of {@link VersionedProcessInstance} that are considered to run on an 'older' process definition | ||
* than the specified newest one. | ||
*/ | ||
public List<VersionedProcessInstance> getOlderProcessInstances(String processDefinitionKey, ProcessVersion newestVersion); |
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.
Public ist in Interfaces eigentlich ja nicht nötig, kommt natürlich auf die eigene Präferenz an aber ich kenne es meistens ohne public.
|
||
import info.novatec.camunda.migrator.ProcessVersion; | ||
|
||
public interface GetOlderProcessInstances { |
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.
Dieses Interface ist im Endeffekt ja gleich zum BiFunction
Interface. Weniger Code ist grundsätzlich glaub ich immer gut allerdings fehlt dann auch der JavaDoc + die sinngebenden Namen. Man kann also für beides argumentieren.
Update: Da bei den anderen Interfaces ja mal vier Parameter gebraucht werden und es sowas wie QuattroFunction ja nicht gibt spricht wohl auch Konsistenz gegen eine Änderung 😄 .
*/ | ||
public List<MinorMigrationInstructions> getApplicableMinorMigrationInstructions(String processDefinitionKey, | ||
int sourceMinorVersion, int targetMinorVersion, int majorVersion); | ||
} |
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.
Gleicher Kommentar wie oben zum public
* the process instance for which the migration plan is to be generated. | ||
* @return a {@link MigrationPlan} for migration the process instance to the newest version. | ||
*/ | ||
public MigrationPlan migrationPlanByMappingEqualActivityIDs(VersionedDefinitionId newestProcessDefinition, |
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.
Wie bei den anderen Interfaces
@@ -7,9 +7,11 @@ | |||
import org.junit.ClassRule; | |||
import org.junit.Test; | |||
|
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.
Ist der absichtlich noch in Junit4?
@@ -18,6 +19,10 @@ | |||
import org.junit.ClassRule; | |||
import org.junit.Test; | |||
|
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.
Ist der absichtlich noch mit JUnit4?
No description provided.