Skip to content

Address peer feedback.#3015

Merged
IEvangelist merged 2 commits intomicrosoft:mainfrom
IEvangelist:json-bool
Mar 20, 2024
Merged

Address peer feedback.#3015
IEvangelist merged 2 commits intomicrosoft:mainfrom
IEvangelist:json-bool

Conversation

@IEvangelist
Copy link
Copy Markdown
Member

@IEvangelist IEvangelist commented Mar 19, 2024

See #2851 (review)

Microsoft Reviewers: Open in CodeFlow

@IEvangelist IEvangelist requested a review from eerhardt March 19, 2024 20:23
@ghost ghost added the area-codeflow for labeling automated codeflow. intentionally a different color! label Mar 19, 2024
@davidfowl
Copy link
Copy Markdown
Contributor

I think before we merge this, we should verify what it means to have a non string argument. Doesn't this translate into a command line argument anyways? (strings)

@eerhardt
Copy link
Copy Markdown
Member

I think before we merge this, we should verify what it means to have a non string argument. Doesn't this translate into a command line argument anyways? (strings)

I'd be fine with "true" in the manifest (i.e. only supporting JSON strings).

I'm not sure how the current code works today with "{{bool.TrueString}}".

@IEvangelist
Copy link
Copy Markdown
Member Author

I think before we merge this, we should verify what it means to have a non string argument. Doesn't this translate into a command line argument anyways? (strings)

I'd be fine with "true" in the manifest (i.e. only supporting JSON strings).

I'm not sure how the current code works today with "{{bool.TrueString}}".

That was the test code, it evaluated as "SOME_BOOL": "True". I'd assume it's probably pretty common for true/false values. Even STJ doesn't have native support for mapping the string representation of bool to a bool property type. This change adds bool support only, otherwise it's functionally the same as it was before.

@IEvangelist IEvangelist merged commit e1a64f5 into microsoft:main Mar 20, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Apr 20, 2024
@IEvangelist IEvangelist deleted the json-bool branch January 30, 2026 17:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-codeflow for labeling automated codeflow. intentionally a different color!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants