refactor!: Use true/false for command parameters to be more consistent #782

jinlinGuan · 2022-12-23T10:09:57Z

BREAKING CHANGE:

ds-pushevent and ds-returnevent to use true/false instead of yes/no

close #780

Signed-off-by: Ginny Guan [email protected]

If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/go-mod-core-contracts/blob/main/.github/Contributing.md

PR Checklist

Please check if your PR fulfills the following requirements:

I am not introducing a breaking change (if you are, flag in conventional commit message with BREAKING CHANGE: describing the break)
I am not introducing a new dependency (add notes below if you are)
I have added unit tests for the new feature or bug fix (if not, why?)
I have fully tested (add details below) this the new feature or bug fix (if not, why?)
I have opened a PR for the related docs change (if not, why?)
docs: Update docs to use true/false for ds-pushevent and ds-returnevent in 3.0 edgex-docs#917

Testing Instructions

build core-command and device-virtual docker images based on the changes and run docker-compose.yml
The following commands work as expected:

curl http://localhost:59882/api/v2/device/name/Random-Boolean-Device/Bool\?ds-pushevent\=true
curl http://localhost:59882/api/v2/device/name/Random-Boolean-Device/Bool\?ds-pushevent\=false
curl http://localhost:59882/api/v2/device/name/Random-Boolean-Device/Bool\?ds-return\=true
curl http://localhost:59882/api/v2/device/name/Random-Boolean-Device/Bool\?ds-return\=false

New Dependency Instructions (If applicable)

codecov-commenter · 2022-12-23T10:19:05Z

Codecov Report

Merging #782 (a2a49d7) into main (7e0c83d) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #782   +/-   ##
=======================================
  Coverage   64.70%   64.70%           
=======================================
  Files          81       81           
  Lines        3108     3108           
=======================================
  Hits         2011     2011           
  Misses        907      907           
  Partials      190      190

Impacted Files	Coverage Δ
clients/http/command.go	`74.46% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

lenny-goodell · 2022-12-23T19:07:41Z

clients/interfaces/command.go

-	// dsPushEvent: If set to yes, a successful GET will result in an event being pushed to the EdgeX system. Default value is no.
-	// dsReturnEvent: If set to no, there will be no Event returned in the http response. Default value is yes.
+	// dsPushEvent: If set to true, a successful GET will result in an event being pushed to the EdgeX system. Default value is false.
+	// dsReturnEvent: If set to false, there will be no Event returned in the http response. Default value is true.
 	IssueGetCommandByName(ctx context.Context, deviceName string, commandName string, dsPushEvent string, dsReturnEvent string) (*responses.EventResponse, errors.EdgeX)


Can we make this interface use bool instead of string for these options?

Suggested change

IssueGetCommandByName(ctx context.Context, deviceName string, commandName string, dsPushEvent string, dsReturnEvent string) (*responses.EventResponse, errors.EdgeX)

IssueGetCommandByName(ctx context.Context, deviceName string, commandName string, dsPushEvent bool, dsReturnEvent bool) (*responses.EventResponse, errors.EdgeX)

lenny-goodell · 2022-12-23T19:08:41Z

common/constants.go

 	ValueTrue      = "true"
 	ValueFalse     = "false"


Do we really need these constants for true and false`??

Suggested change

ValueTrue = "true"

ValueFalse = "false"

ValueTrue = true

ValueFalse = false

yes, we still need them for the query parameter definition to validate the values

they should still be in string data type, because the REST query parameter only allows string value

Ok, can we then parse them to bool so that the IssueGetCommandByName func can use bool?

Sure, IssueGetCommandByName client can use bool directly, and ValueTrue/ValueFalse will be used in the core-command internal implementation.

lenny-goodell · 2022-12-23T19:13:48Z

Please change commit message to have breaking change all on one line. The Conventional Commit spec what it all one one line 2 blank line???. I think 2 blank line is used when there is a very long description. I missed this on the other PR :-( .
https://github.com/edgexfoundry/go-mod-core-contracts/blob/main/.github/Contributing.md#footer

BREAKING CHANGE: ds-pushevent and ds-returnevent to use true/false instead of yes/no

BREAKING CHANGE: ds-pushevent and ds-returnevent to use true/false instead of yes/no close edgexfoundry#780 Signed-off-by: Ginny Guan <[email protected]>

lenny-goodell

LGTM

…Name BREAKING CHANGE: ds-pushevent and ds-returnevent to use bool true/false instead of yes/no related: edgexfoundry/go-mod-core-contracts#782 Signed-off-by: Ginny Guan <[email protected]>

cloudxxx8

LGTM

BREAKING CHANGE: ds-pushevent and ds-returnevent to use bool true/false instead of yes/no related: edgexfoundry/go-mod-core-contracts#782 Signed-off-by: Ginny Guan <[email protected]>

#4276) * refactor!: Use bool types for command parameters to be more consistent BREAKING CHANGE: ds-pushevent and ds-returnevent to use bool true/false instead of yes/no related: edgexfoundry/go-mod-core-contracts#782 Signed-off-by: Ginny Guan <[email protected]>

BREAKING CHANGE: ds-pushevent and ds-returnevent to use bool true/false instead of yes/no related: edgexfoundry/go-mod-core-contracts#782 Signed-off-by: Ginny Guan <[email protected]>

lenny-goodell requested changes Dec 23, 2022

View reviewed changes

refactor!: Use true/false for command parameters to be more consistent

a2a49d7

BREAKING CHANGE: ds-pushevent and ds-returnevent to use true/false instead of yes/no close edgexfoundry#780 Signed-off-by: Ginny Guan <[email protected]>

jinlinGuan force-pushed the issue-780 branch from 946c974 to a2a49d7 Compare January 11, 2023 17:03

jinlinGuan requested a review from lenny-goodell January 11, 2023 17:04

lenny-goodell approved these changes Jan 11, 2023

View reviewed changes

cloudxxx8 approved these changes Jan 12, 2023

View reviewed changes

cloudxxx8 merged commit 8b66e6c into edgexfoundry:main Jan 12, 2023

jinlinGuan mentioned this pull request Jan 12, 2023

refactor!: Use bool types for command parameters to be more consistent edgexfoundry/go-mod-messaging#207

Merged

5 tasks

jinlinGuan deleted the issue-780 branch January 12, 2023 10:27

jinlinGuan mentioned this pull request Jan 12, 2023

refactor!: Use bool types for command parameters to be more consistent edgexfoundry/device-sdk-go#1279

Merged

5 tasks

jinlinGuan mentioned this pull request Jan 12, 2023

refactor!: Use bool types for command parameters to be more consistent edgexfoundry/edgex-go#4276

Merged

5 tasks

cherrycl mentioned this pull request Jan 13, 2023

Update ds-pushevent and ds-returnevent to use true/false instead of yes/no edgexfoundry/edgex-taf#763

Closed

iain-anderson mentioned this pull request Jan 16, 2023

refactor: V3 name changes edgexfoundry/device-sdk-c#437

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: Use true/false for command parameters to be more consistent #782

refactor!: Use true/false for command parameters to be more consistent #782

jinlinGuan commented Dec 23, 2022 •

edited

Loading

codecov-commenter commented Dec 23, 2022 •

edited

Loading

lenny-goodell Dec 23, 2022

lenny-goodell Dec 23, 2022

cloudxxx8 Jan 3, 2023 •

edited

Loading

cloudxxx8 Jan 3, 2023

lenny-goodell Jan 3, 2023

cloudxxx8 Jan 3, 2023

lenny-goodell commented Dec 23, 2022

lenny-goodell left a comment

cloudxxx8 left a comment

	IssueGetCommandByName(ctx context.Context, deviceName string, commandName string, dsPushEvent string, dsReturnEvent string) (*responses.EventResponse, errors.EdgeX)
	IssueGetCommandByName(ctx context.Context, deviceName string, commandName string, dsPushEvent bool, dsReturnEvent bool) (*responses.EventResponse, errors.EdgeX)

refactor!: Use true/false for command parameters to be more consistent #782

refactor!: Use true/false for command parameters to be more consistent #782

Conversation

jinlinGuan commented Dec 23, 2022 • edited Loading

PR Checklist

Testing Instructions

New Dependency Instructions (If applicable)

codecov-commenter commented Dec 23, 2022 • edited Loading

Codecov Report

lenny-goodell Dec 23, 2022

Choose a reason for hiding this comment

lenny-goodell Dec 23, 2022

Choose a reason for hiding this comment

cloudxxx8 Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

cloudxxx8 Jan 3, 2023

Choose a reason for hiding this comment

lenny-goodell Jan 3, 2023

Choose a reason for hiding this comment

cloudxxx8 Jan 3, 2023

Choose a reason for hiding this comment

lenny-goodell commented Dec 23, 2022

lenny-goodell left a comment

Choose a reason for hiding this comment

cloudxxx8 left a comment

Choose a reason for hiding this comment

jinlinGuan commented Dec 23, 2022 •

edited

Loading

codecov-commenter commented Dec 23, 2022 •

edited

Loading

cloudxxx8 Jan 3, 2023 •

edited

Loading