Add API to retrive all topic related data from the change #77
Conversation
| */ | ||
| private String topic; | ||
| private Topic topic; |
There was a problem hiding this comment.
This is dangerous as old serialized data will fail to load (for example old build records in Jenkins).
The old field unfortunately needs to stay.
You can mark it as transient so that new objects won't serialize the data. And add a readResolve method to convert the old field to the new one if possible.
| * @param topic the topic. | ||
| */ | ||
| public void setTopic(String topic) { | ||
| public void setTopic(Topic topic) { |
There was a problem hiding this comment.
breaking binary backwards compatibility.
| @@ -41,7 +42,7 @@ | |||
| /** | |||
| * The previous topic value. New topic is in change.topic. | |||
| */ | |||
| private String oldTopic; | |||
| private Topic oldTopic; | |||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
What is the rational of moving this?
There was a problem hiding this comment.
To be able to query list of files outside of ChangeBasedEvent. i.e.
for(Change change : change.getTopic().getChanges()) {
List<String> files = change.getFiles(gerritQueryHandler);
// do something with this files
}
Jimilian
force-pushed
the
native_topic
branch
3 times, most recently
from
42a23d4
to
1cadefe
Compare
|
@rsandell Done. Please take a look. |
Jimilian
force-pushed
the
native_topic
branch
from
1cadefe
to
29b272f
Compare
Also adds change status to the change based events.
Jimilian
force-pushed
the
native_topic
branch
from
29b272f
to
6f507cb
Compare
| */ | ||
| UNKNOWN("UNKNOWN"); | ||
|
|
||
| private final String status; |
There was a problem hiding this comment.
nit The status is a bit redundant since GerritChangeStatus.name() will give the same value.
There was a problem hiding this comment.
yes, you are right
| /** | ||
| * The changed files in this patchset. | ||
| */ | ||
| private List<String> files; |
There was a problem hiding this comment.
Maybe I missed this one before, but this is also where old data will fail to serialize, so you need to do something similar to Topic. I.e. keep the old field but make it transient and add a readResolve that'll move the files into Change.
| */ | ||
| public Map<Change, PatchSet> getChanges(GerritQueryHandler gerritQueryHandler) { | ||
| if (changes == null) { | ||
| changes = new HashMap<Change, PatchSet>(); |
There was a problem hiding this comment.
If something fails below, you will end up with an empty change list in this topic "forever" And will not be able to retrieve them on a second try.
Jimilian
force-pushed
the
native_topic
branch
from
959a2da
to
d5dbd19
Compare
|
@rsandell Done. This time I tried to simplify review as much as possible :) |
| } | ||
| } | ||
| return files; | ||
| return change.getFiles(gerritQueryHandler); |
There was a problem hiding this comment.
Since the old behavior has changed in FileHelper.getFilesByChange (it now returns null instead of an empty list), just to be safe I think we need to preserve the old behavior here. E.g. if returned == null { return Collections.emptyList() }
Jimilian
force-pushed
the
native_topic
branch
from
d5dbd19
to
75961a7
Compare
|
All your points were valid. So, I'm completely fine to fix it :) Last change is amended. |
Also adds change status to the change based events.