-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Expand environment variables for sparse checkout path #1066
base: master
Are you sure you want to change the base?
Conversation
|
||
public class SparseCheckoutPath extends AbstractDescribableImpl<SparseCheckoutPath> implements Serializable { | ||
|
||
private static final long serialVersionUID = -6177158367915899356L; | ||
|
||
@SuppressFBWarnings(value="SE_TRANSIENT_FIELD_NOT_RESTORED", justification="Default value is OK in deserialization") | ||
public static final transient SparseCheckoutPathToPath SPARSE_CHECKOUT_PATH_TO_PATH = new SparseCheckoutPathToPath(); |
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.
Won't this change break compatibility for git plugin API consumers that refer to this public field that is being deleted?
It is included in the Javadoc of the current release.
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.
Updated to keep this variable for API consumers.
public String apply(@NonNull SparseCheckoutPath sparseCheckoutPath) { | ||
return sparseCheckoutPath.getPath(); | ||
} | ||
public Descriptor<SparseCheckoutPath> getDescriptor() { |
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.
Please don't make white space changes in the git plugin. The formatting is ugly and inconsistent, but changing white space makes it more difficult to read the changes and more difficult to interact with other proposed changes in the same file.
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.
I don't understand why the getDescriptor() call moved outside SparseCheckoutPathToPath
. What makes that move necessary?
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.
This was not intentional. Somehow changed on reformating the code in IDE.
} | ||
|
||
@Extension | ||
public static class DescriptorImpl extends Descriptor<SparseCheckoutPath> { | ||
@Override | ||
public String getDisplayName() { return "Path"; } |
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.
Please don't make white space only changes in the git plugin. See the contributing file for more details.
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.
As mentioned this was not intentional either.
@Extension | ||
public static class DescriptorImpl extends GitSCMExtensionDescriptor { | ||
@Override | ||
public String getDisplayName() { | ||
return "Sparse Checkout paths"; | ||
} |
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.
Why moving so many methods outside the descriptor implementation class? Seems like that will break many things and add unused public methods to the SparseCheckoutPaths object.
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.
Same
@@ -67,11 +68,11 @@ public boolean equals(Object o) { | |||
if (this == o) { | |||
return true; | |||
} | |||
|
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.
I am not able to resolve these issues.
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.
I am not able to revert these changes.
JENKINS-23477
Expand environment variables in sparse checkout path using pattern matching to look for environment variables name.
Checklist
Types of changes