Skip to content
This repository has been archived by the owner on Nov 13, 2023. It is now read-only.

Array with allowable values #429

Merged
3 commits merged into from
Aug 6, 2020
Merged

Conversation

liuxh0
Copy link
Contributor

@liuxh0 liuxh0 commented Jul 23, 2020

liuxh0 added 2 commits July 23, 2020 17:20
Signed-off-by: Xinhu Liu <xinhu.liu90@gmail.com>
Signed-off-by: Xinhu Liu <xinhu.liu90@gmail.com>
@codecov
Copy link

codecov bot commented Jul 23, 2020

Codecov Report

Merging #429 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #429      +/-   ##
==========================================
- Coverage   81.82%   81.80%   -0.02%     
==========================================
  Files         157      157              
  Lines        7764     7613     -151     
  Branches     1419     1374      -45     
==========================================
- Hits         6353     6228     -125     
+ Misses       1407     1381      -26     
  Partials        4        4              
Impacted Files Coverage Δ
packages/cmd/src/syntax/SyntaxValidator.ts 80.05% <100.00%> (-0.80%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55d6e57...cd3e4ed. Read the comment docs.

@zFernand0 zFernand0 linked an issue Jul 30, 2020 that may be closed by this pull request
@ghost
Copy link

ghost commented Jul 31, 2020

LGTM. Please merge the latest changes from master into your branch.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 1, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (1.8.0_212) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

@scifipony
Copy link

Please ensure in testing that an array of no elements (an option with no argument) is not flagged in error if allowableValues is not coded, or this will be a breaking change.

@liuxh0
Copy link
Contributor Author

liuxh0 commented Aug 2, 2020

Hi @scifipony , I am not really sure what you mean.

The scenario you described will be flagged in error because an array is expected but a boolean is provided. The behavior is exactly the same before my change. Why this is a breaking change?

However in Zowe, an array-type option with no argument will result in an empty array instead of a boolean true. I am not familiar but I assume Zowe did some error handling.

@scifipony
Copy link

@liuxh0

in Zowe, an array-type option with no argument will result in an empty array instead of a boolean true.

Correct.

I assume Zowe did some error handling

Best I can tell, it does not. It certainly does not flag as in-error an array-type option with no argument. I want to ensure that this behavior continues. Should Zowe start flagging it, it would be a breaking change for my product.

@liuxh0
Copy link
Contributor Author

liuxh0 commented Aug 2, 2020

@scifipony
I am still a little bit confused. Before and after, the behavior is the same. How can I help here?

@scifipony
Copy link

@scifipony
I am still a little bit confused. Before and after, the behavior is the same. How can I help here?

All I was asking to ensure the behavior didn't change. If the behavior is the same "before and after" the changes you made, then everything is fine.

Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

LGTM

@ghost ghost merged commit 95f1b93 into zowe:master Aug 6, 2020
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allowableValues doesn't work with array type
3 participants