Refactor for clarity and consistency #154
Conversation
| revisionActions | ||
| ); | ||
| } else { | ||
| listener.getLogger().format("No automatic build triggered for %s%n", rawName); |
There was a problem hiding this comment.
Changed the wording here from "No automatic builds for ...".
| * @param lastSeenRevision the last seen revision | ||
| * @return {@code true} if the head should be automatically built when discovered / modified. | ||
| */ | ||
| private boolean isAutomaticBuild(@NonNull SCMHead head, |
There was a problem hiding this comment.
Moved this method into the observer class. There is not advantage to it being on the project instance.
| SCMRevision scmLastSeenRevision = getLastSeenRevision(project, scmLastBuiltRevision); | ||
| if (!revision.equals(scmLastBuiltRevision)) { | ||
|
|
||
| if (isChangesDetected(revision, project, scmLastBuiltRevision)) { |
There was a problem hiding this comment.
Instead of running two almost identical blocks, the checks are handled by isChangesDetected and then the actual same code path is executed.
| } else { | ||
| // get the previous revision | ||
| SCMRevision scmLastBuiltRevision = _factory.getRevision(project); | ||
|
|
||
| if (isChangesDetected(revision, project, scmLastBuiltRevision)) { | ||
| listener.getLogger() | ||
| .format("Changes detected: %s (%s → %s)%n", rawName, scmLastBuiltRevision, revision); |
There was a problem hiding this comment.
Moved this up to make it extra clear that is doesn't use scmLastSeenRevision .
| try { | ||
| branch.setActions(source.fetchActions(head, event, listener)); | ||
| headActionsFetched = true; |
There was a problem hiding this comment.
The only reason headActionsFetched existed was so that we could try an alternative later.
Go ahead and do it in place.
| if (isAutomaticBuild(head, revision, scmLastBuiltRevision, scmLastSeenRevision)) { | ||
| Action[] revisionActions = getRevisionActions(revision, rawName); |
There was a problem hiding this comment.
We only need revision actions when we're ready to schedule a build.
| } | ||
| try { | ||
| _factory.setLastSeenRevisionHash(project, revision); | ||
| _factory.setLastSeenRevisionHash(project, scmLastBuiltRevision); |
There was a problem hiding this comment.
A getter that mutates data isn't very intuitive at first glance. It's a private method, so not a big deal. But maybe just call it lastSeenRevision(...) for a tiny bit ore clarity?
Co-Authored-By: Robert Sandell <rsandell@cloudbees.com>
After #152 and #153, I decided to refactor this huge method.
This is almost pure refactor, but not quite because of minor bugs I found while refactoring.
I will call out the non refactor changes
It will be useful to view the diffs for each commit and also to set your diff settings to "Hide whitespace differences".
Depends on #153.