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.

Sign up for GitHub

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

Jump to bottom

Allow null param map #43

Merged
merged 4 commits into from
Jan 4, 2019
Merged

Allow null param map #43

merged 4 commits into from
Jan 4, 2019

Conversation

Dhivyaa21
Copy link
Contributor

@Dhivyaa21 Dhivyaa21 commented Jan 2, 2019 •
edited

Issue #42

@Dhivyaa21
Copy link
Contributor Author

@cdancy - Done with the changes. Please review

martinda
martinda approved these changes Jan 4, 2019
View reviewed changes
Copy link
Collaborator

@martinda martinda left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Dhivyaa21
Copy link
Contributor Author

@martinda Thanks. Can we go ahead and close the PR?

@martinda
Copy link
Collaborator

martinda commented Jan 4, 2019

I ran the mock and live tests locally and everything passes.

@martinda martinda self-assigned this Jan 4, 2019
cdancy
cdancy approved these changes Jan 4, 2019
View reviewed changes
Copy link
Owner

@cdancy cdancy left a comment

Choose a reason for hiding this comment

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

LGTM

@martinda martinda merged commit 31235c3 into cdancy:master Jan 4, 2019
@cdancy
Copy link
Owner

cdancy commented Jan 4, 2019

@martinda actually, and prior to releasing, I wonder if we should also support an empty map here? On the face of things it seems weird to support a null map but not an empty one.

@cdancy cdancy added this to the v0.0.15 milestone Jan 4, 2019
@martinda
Copy link
Collaborator

martinda commented Jan 4, 2019

@cdancy Yeah... I missed that feature:

jenkins-rest/src/main/java/com/cdancy/jenkins/rest/binders/BindMapToForm.java

Line 43 in 0cf25b7

checkArgument(props.size() > 0, "properties Map cannot be empty");

@Dhivyaa21 do you mind sending another pull-request please?

@Dhivyaa21
Copy link
Contributor Author

Sure. I can send another one.

@cdancy cdancy modified the milestones: v0.0.15, v0.0.16 Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinda martinda martinda approved these changes

@cdancy cdancy cdancy approved these changes

Assignees

@martinda martinda

Labels
None yet
Projects
None yet
Milestone
v0.0.16
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Dhivyaa21 @martinda @cdancy