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

Allow boolean and number parameters types in command requests #446

Merged
merged 2 commits into from Jan 9, 2019

Conversation

Projects
None yet
2 participants
@dansmithy
Copy link
Contributor

commented Jan 9, 2019

No description provided.

@dansmithy dansmithy requested a review from cdupuis Jan 9, 2019

@cdupuis
Copy link
Contributor

left a comment

There are actually quite a more changes required to make that work properly (including tests which are missing from this PR and changes to the possible Arg values which can only be strings right now).

I have most of these changes pending on a local branch which also already includes the addition from this PR.

Note: Also the commit message and PR title indicate that this is a fix which IMHO it isn't. It is actually part of a completely new feature that was never really implemented: Support for other types in command handler parameter values besides strings. Hence I'd recommend to rename the commit and PR and also remove the dot at the end of the commit message.

@dansmithy

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

@cdupuis Sounds like a) I've misunderstood a few things, and b) you have something in progress. Should I just drop this change and wait for your enhancement? Happy to chat if that is easier.

@cdupuis

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

I'll rebase my changes onto your branch and push. Then you can review.

@cdupuis cdupuis changed the title Fixing type coercion for numbers. Allow boolean and number parameters types in command requests Jan 9, 2019

@cdupuis

cdupuis approved these changes Jan 9, 2019

@cdupuis cdupuis force-pushed the djs-number-params-ok branch from e05dc57 to 3cef180 Jan 9, 2019

@@ -57,22 +58,24 @@ function computeValue(parameter: { name: string, type?: ParameterType }, value:
break;
case "boolean":
if (typeof value !== "boolean") {
value = value === "true" || value === "yes" || value === "1";
newValue = value === "true" || value === "yes" || value === "1";
}
break;
case "number":
if (typeof value === "string") {
value = parseInt(value, 10);

This comment has been minimized.

Copy link
@dansmithy

dansmithy Jan 9, 2019

Author Contributor

surely this should be newValue = ... does the below test really pass? (I can make this change if not)

This comment has been minimized.

Copy link
@cdupuis

cdupuis Jan 9, 2019

Contributor

Yeah, that was a rebase/merge error that I didn't catch.

@dansmithy
Copy link
Contributor Author

left a comment

@cdupuis LGTM, but I can't approve as it is my PR.

@cdupuis cdupuis merged commit a776dc9 into master Jan 9, 2019

2 checks passed

license/cla Contributor License Agreement is signed.
Details
sdm/atomist/atomist-sdm Atomist Software Delivery Machine goals: all succeeded
Details

@cdupuis cdupuis deleted the djs-number-params-ok branch Jan 9, 2019

atomist-bot added a commit that referenced this pull request Jan 9, 2019

Changelog: #446 to added
[atomist:generated]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.