Skip to content
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

Jenkins 19082 #898

Closed
wants to merge 2 commits into from
Closed

Conversation

arangamani
Copy link
Member

This fixes the includeRegex parameter in ListView. There was a bug introduced in 1.526 where whatever the regex is given in the configuration is replaced by "on" as a wrong Stapler request parameter was used to compile the regex pattern.

It fixes: https://issues.jenkins-ci.org/browse/JENKINS-19082

@olivergondza
Copy link
Member

Hi, thanks for contributing.

The commit that introduced the regression was already released. It introduced a new public API member ListView.setIncludeRegex(String) and the fix you propose removes it. I guess you can safely add

public void setIncludeRegex(String includeRegex) {
    setIncludeRegex(includeRegex, includeRegex);
}

to retain compatibility.

Please, try to avoid unnecessary backspace changes and include issue references to commit messages as suggested in https://wiki.jenkins-ci.org/display/JENKINS/contributing#contributing-CommitMessages.

@arangamani
Copy link
Member Author

Oliver,

Sure. I can add that method to be backward compatible but that will still not work though.

In the coding style of any programming languages that I've used, I learned to strip trailing white spaces. Since I have a setting in my editor to remove that automatically, it stripped them off. I know it is hard to review because of so many changes. I will put those trailing white spaces back! I've already edited the commit messages to match the style and follow the same standard in the future.

Thanks for the review.

public void setIncludeRegex(String includeRegex) {
if (includeRegex != null) {

public void setIncludeRegex(String useIncludeRegex, String includeRegex) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is not meant to know it is called from a form submission, the signature change is unnecessary.

@Vlatombe
Copy link
Member

Vlatombe commented Aug 8, 2013

Good catch about the parameter names, I've provided some feedback to your pull request.

@jglick
Copy link
Member

jglick commented Aug 8, 2013

I have a setting in my editor to remove that automatically

Good editors can be configured to only strip whitespace from lines you were already editing.

@Vlatombe
Copy link
Member

I think this can be closed as I submitted #905 which has already been merged and is fixing the issue.

@jglick jglick closed this Aug 14, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants