From b0bbe0f7588fcc102f00fa9ff699ef0cd2969cb2 Mon Sep 17 00:00:00 2001 From: Ivan Fernandez Calvo Date: Tue, 26 Jan 2021 16:14:33 +0100 Subject: [PATCH 1/2] fix: use constants not magic numbers --- .../BranchDiscoveryTrait.java | 35 ++++++++++++++----- .../ForkPullRequestDiscoveryTrait.java | 32 ++++++++++++----- .../OriginPullRequestDiscoveryTrait.java | 33 ++++++++++++----- .../GitHubSCMNavigatorTraitsTest.java | 2 +- .../GitHubSCMSourceTraitsTest.java | 2 +- 5 files changed, 77 insertions(+), 27 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/BranchDiscoveryTrait.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/BranchDiscoveryTrait.java index 37d7727d1..798dc98cf 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/BranchDiscoveryTrait.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/BranchDiscoveryTrait.java @@ -52,6 +52,23 @@ * @since 2.2.0 */ public class BranchDiscoveryTrait extends SCMSourceTrait { + /** + * None strategy. + */ + public static final int NONE = 0; + /** + * Exclude branches that are also filed as PRs. + */ + public static final int EXCLUDE_PRS = 1; + /** + * Only branches that are also filed as PRs. + */ + public static final int ONLYP_RS = 2; + /** + * All branches. + */ + public static final int ALL_BRANCHES = 3; + /** * The strategy encoded as a bit-field. */ @@ -74,7 +91,7 @@ public BranchDiscoveryTrait(int strategyId) { * @param buildBranchWithPr build branches that are also PRs. */ public BranchDiscoveryTrait(boolean buildBranch, boolean buildBranchWithPr) { - this.strategyId = (buildBranch ? 1 : 0) + (buildBranchWithPr ? 2 : 0); + this.strategyId = (buildBranch ? EXCLUDE_PRS : NONE) + (buildBranchWithPr ? ONLYP_RS : NONE); } /** @@ -93,7 +110,7 @@ public int getStrategyId() { */ @Restricted(NoExternalUse.class) public boolean isBuildBranch() { - return (strategyId & 1) != 0; + return (strategyId & EXCLUDE_PRS) != NONE; } /** @@ -103,7 +120,7 @@ public boolean isBuildBranch() { */ @Restricted(NoExternalUse.class) public boolean isBuildBranchesWithPR() { - return (strategyId & 2) != 0; + return (strategyId & ONLYP_RS) != NONE; } /** @@ -115,15 +132,15 @@ protected void decorateContext(SCMSourceContext context) { ctx.wantBranches(true); ctx.withAuthority(new BranchSCMHeadAuthority()); switch (strategyId) { - case 1: + case BranchDiscoveryTrait.EXCLUDE_PRS: ctx.wantOriginPRs(true); ctx.withFilter(new ExcludeOriginPRBranchesSCMHeadFilter()); break; - case 2: + case BranchDiscoveryTrait.ONLYP_RS: ctx.wantOriginPRs(true); ctx.withFilter(new OnlyOriginPRBranchesSCMHeadFilter()); break; - case 3: + case BranchDiscoveryTrait.ALL_BRANCHES: default: // we don't care if it is a PR or not, we're taking them all, no need to ask for PRs and no need // to filter @@ -181,9 +198,9 @@ public Class getSourceClass() { @SuppressWarnings("unused") // stapler public ListBoxModel doFillStrategyIdItems() { ListBoxModel result = new ListBoxModel(); - result.add(Messages.BranchDiscoveryTrait_excludePRs(), "1"); - result.add(Messages.BranchDiscoveryTrait_onlyPRs(), "2"); - result.add(Messages.BranchDiscoveryTrait_allBranches(), "3"); + result.add(Messages.BranchDiscoveryTrait_excludePRs(), String.valueOf(EXCLUDE_PRS)); + result.add(Messages.BranchDiscoveryTrait_onlyPRs(), String.valueOf(ONLYP_RS)); + result.add(Messages.BranchDiscoveryTrait_allBranches(), String.valueOf(ALL_BRANCHES)); return result; } } diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait.java index cff2da514..0e07170ce 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait.java @@ -56,6 +56,22 @@ * @since 2.2.0 */ public class ForkPullRequestDiscoveryTrait extends SCMSourceTrait { + /** + * None strategy. + */ + public static final int NONE = 0; + /** + * Merging the pull request with the current target branch revision. + */ + public static final int MERGE = 1; + /** + * The current pull request revision. + */ + public static final int HEAD = 2; + /** + * Both the current pull request revision and the pull request merged with the current target branch revision. + */ + public static final int HEAD_AND_MERGE = 3; /** * The strategy encoded as a bit-field. */ @@ -87,8 +103,8 @@ public ForkPullRequestDiscoveryTrait(int strategyId, */ public ForkPullRequestDiscoveryTrait(@NonNull Set strategies, @NonNull SCMHeadAuthority trust) { - this((strategies.contains(ChangeRequestCheckoutStrategy.MERGE) ? 1 : 0) - + (strategies.contains(ChangeRequestCheckoutStrategy.HEAD) ? 2 : 0), trust); + this((strategies.contains(ChangeRequestCheckoutStrategy.MERGE) ? MERGE : NONE) + + (strategies.contains(ChangeRequestCheckoutStrategy.HEAD) ? HEAD : NONE), trust); } /** @@ -108,11 +124,11 @@ public int getStrategyId() { @NonNull public Set getStrategies() { switch (strategyId) { - case 1: + case ForkPullRequestDiscoveryTrait.MERGE: return EnumSet.of(ChangeRequestCheckoutStrategy.MERGE); - case 2: + case ForkPullRequestDiscoveryTrait.HEAD: return EnumSet.of(ChangeRequestCheckoutStrategy.HEAD); - case 3: + case ForkPullRequestDiscoveryTrait.HEAD_AND_MERGE: return EnumSet.of(ChangeRequestCheckoutStrategy.HEAD, ChangeRequestCheckoutStrategy.MERGE); default: return EnumSet.noneOf(ChangeRequestCheckoutStrategy.class); @@ -190,9 +206,9 @@ public Class getSourceClass() { @SuppressWarnings("unused") // stapler public ListBoxModel doFillStrategyIdItems() { ListBoxModel result = new ListBoxModel(); - result.add(Messages.ForkPullRequestDiscoveryTrait_mergeOnly(), "1"); - result.add(Messages.ForkPullRequestDiscoveryTrait_headOnly(), "2"); - result.add(Messages.ForkPullRequestDiscoveryTrait_headAndMerge(), "3"); + result.add(Messages.ForkPullRequestDiscoveryTrait_mergeOnly(), String.valueOf(MERGE)); + result.add(Messages.ForkPullRequestDiscoveryTrait_headOnly(), String.valueOf(HEAD)); + result.add(Messages.ForkPullRequestDiscoveryTrait_headAndMerge(), String.valueOf(HEAD_AND_MERGE)); return result; } diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/OriginPullRequestDiscoveryTrait.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/OriginPullRequestDiscoveryTrait.java index 17051b6a7..b0ecd1978 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/OriginPullRequestDiscoveryTrait.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/OriginPullRequestDiscoveryTrait.java @@ -54,6 +54,23 @@ * @since 2.2.0 */ public class OriginPullRequestDiscoveryTrait extends SCMSourceTrait { + /** + * None strategy. + */ + public static final int NONE = 0; + /** + * Merging the pull request with the current target branch revision. + */ + public static final int MERGE = 1; + /** + * The current pull request revision. + */ + public static final int HEAD = 2; + /** + * Both the current pull request revision and the pull request merged with the current target branch revision. + */ + public static final int HEAD_AND_MERGE = 3; + /** * The strategy encoded as a bit-field. */ @@ -75,8 +92,8 @@ public OriginPullRequestDiscoveryTrait(int strategyId) { * @param strategies the {@link ChangeRequestCheckoutStrategy} instances. */ public OriginPullRequestDiscoveryTrait(Set strategies) { - this((strategies.contains(ChangeRequestCheckoutStrategy.MERGE) ? 1 : 0) - + (strategies.contains(ChangeRequestCheckoutStrategy.HEAD) ? 2 : 0)); + this((strategies.contains(ChangeRequestCheckoutStrategy.MERGE) ? MERGE : NONE) + + (strategies.contains(ChangeRequestCheckoutStrategy.HEAD) ? HEAD : NONE)); } /** @@ -96,11 +113,11 @@ public int getStrategyId() { @NonNull public Set getStrategies() { switch (strategyId) { - case 1: + case OriginPullRequestDiscoveryTrait.MERGE: return EnumSet.of(ChangeRequestCheckoutStrategy.MERGE); - case 2: + case OriginPullRequestDiscoveryTrait.HEAD: return EnumSet.of(ChangeRequestCheckoutStrategy.HEAD); - case 3: + case OriginPullRequestDiscoveryTrait.HEAD_AND_MERGE: return EnumSet.of(ChangeRequestCheckoutStrategy.HEAD, ChangeRequestCheckoutStrategy.MERGE); default: return EnumSet.noneOf(ChangeRequestCheckoutStrategy.class); @@ -168,9 +185,9 @@ public Class getSourceClass() { @SuppressWarnings("unused") // stapler public ListBoxModel doFillStrategyIdItems() { ListBoxModel result = new ListBoxModel(); - result.add(Messages.ForkPullRequestDiscoveryTrait_mergeOnly(), "1"); - result.add(Messages.ForkPullRequestDiscoveryTrait_headOnly(), "2"); - result.add(Messages.ForkPullRequestDiscoveryTrait_headAndMerge(), "3"); + result.add(Messages.ForkPullRequestDiscoveryTrait_mergeOnly(), String.valueOf(MERGE)); + result.add(Messages.ForkPullRequestDiscoveryTrait_headOnly(), String.valueOf(HEAD)); + result.add(Messages.ForkPullRequestDiscoveryTrait_headAndMerge(), String.valueOf(HEAD_AND_MERGE)); return result; } } diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTraitsTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTraitsTest.java index 61a26f3fd..da844b8b9 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTraitsTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTraitsTest.java @@ -1641,7 +1641,7 @@ public void given__instance__when__setTraits_empty__then__traitsEmpty() { @Test public void given__instance__when__setTraits__then__traitsSet() { GitHubSCMNavigator instance = new GitHubSCMNavigator("test"); - instance.setTraits(Arrays.asList(new BranchDiscoveryTrait(1), + instance.setTraits(Arrays.asList(new BranchDiscoveryTrait(BranchDiscoveryTrait.EXCLUDE_PRS), new SSHCheckoutTrait(null))); assertThat(instance.getTraits(), containsInAnyOrder( diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSourceTraitsTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSourceTraitsTest.java index cde36937c..ddd0e531d 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSourceTraitsTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSourceTraitsTest.java @@ -540,7 +540,7 @@ public void given__legacyCode__when__setBuildOriginBranch__then__traitsMaintaine @Test public void given__instance__when__setTraits__then__traitsSet() { GitHubSCMSource instance = new GitHubSCMSource("testing", "test-repo"); - instance.setTraits(Arrays.asList(new BranchDiscoveryTrait(1), + instance.setTraits(Arrays.asList(new BranchDiscoveryTrait(BranchDiscoveryTrait.EXCLUDE_PRS), new SSHCheckoutTrait("value"))); assertThat(instance.getTraits(), containsInAnyOrder( From ec747ce8d88910392fe2d8228f40e25b18a1d1a2 Mon Sep 17 00:00:00 2001 From: Ivan Fernandez Calvo Date: Tue, 26 Jan 2021 18:27:25 +0100 Subject: [PATCH 2/2] fix: typo --- .../github_branch_source/BranchDiscoveryTrait.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/BranchDiscoveryTrait.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/BranchDiscoveryTrait.java index 798dc98cf..376f68116 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/BranchDiscoveryTrait.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/BranchDiscoveryTrait.java @@ -63,7 +63,7 @@ public class BranchDiscoveryTrait extends SCMSourceTrait { /** * Only branches that are also filed as PRs. */ - public static final int ONLYP_RS = 2; + public static final int ONLY_PRS = 2; /** * All branches. */ @@ -91,7 +91,7 @@ public BranchDiscoveryTrait(int strategyId) { * @param buildBranchWithPr build branches that are also PRs. */ public BranchDiscoveryTrait(boolean buildBranch, boolean buildBranchWithPr) { - this.strategyId = (buildBranch ? EXCLUDE_PRS : NONE) + (buildBranchWithPr ? ONLYP_RS : NONE); + this.strategyId = (buildBranch ? EXCLUDE_PRS : NONE) + (buildBranchWithPr ? ONLY_PRS : NONE); } /** @@ -120,7 +120,7 @@ public boolean isBuildBranch() { */ @Restricted(NoExternalUse.class) public boolean isBuildBranchesWithPR() { - return (strategyId & ONLYP_RS) != NONE; + return (strategyId & ONLY_PRS) != NONE; } /** @@ -136,7 +136,7 @@ protected void decorateContext(SCMSourceContext context) { ctx.wantOriginPRs(true); ctx.withFilter(new ExcludeOriginPRBranchesSCMHeadFilter()); break; - case BranchDiscoveryTrait.ONLYP_RS: + case BranchDiscoveryTrait.ONLY_PRS: ctx.wantOriginPRs(true); ctx.withFilter(new OnlyOriginPRBranchesSCMHeadFilter()); break; @@ -199,7 +199,7 @@ public Class getSourceClass() { public ListBoxModel doFillStrategyIdItems() { ListBoxModel result = new ListBoxModel(); result.add(Messages.BranchDiscoveryTrait_excludePRs(), String.valueOf(EXCLUDE_PRS)); - result.add(Messages.BranchDiscoveryTrait_onlyPRs(), String.valueOf(ONLYP_RS)); + result.add(Messages.BranchDiscoveryTrait_onlyPRs(), String.valueOf(ONLY_PRS)); result.add(Messages.BranchDiscoveryTrait_allBranches(), String.valueOf(ALL_BRANCHES)); return result; }