CLI-172 filter issues by status#165
CLI-172 filter issues by status#165sophio-japharidze-sonarsource wants to merge 1 commit intomasterfrom
Conversation
SummaryThis PR adds support for filtering issues by status via a new Also includes a parameter rename in the internal API contract from What reviewers should knowWhere to start: Check the command definition in command-tree.ts (lines 131-141) to see the option added and how it displays valid values. The validation logic is in issues.ts (lines 80-88). Key decisions:
Test coverage: Integration tests verify validation errors, single-value filtering, and multi-value filtering. The fake server filters by the new parameter correctly. Old unit test updated to use new parameter name.
|
|
![sonar-review-alpha[bot]](https://avatars.githubusercontent.com/in/3025063?s=60&v=4){kind=small}
nquinquenel
left a comment
nquinquenel
left a comment
There was a problem hiding this comment.
Looks good, mostly one important feedback about wrong description or logic
| .option( | ||
| '--severity <severity>', | ||
| `Filter by severity (comma-separated list of: ${VALID_SEVERITIES.join(', ')})`, | ||
| ) |
There was a problem hiding this comment.
Severity moved from single value to comma-separated, but the implementation in issues.ts still treats it as a single value. Should align the implementation
| expect(result.stdout).toContain('"total": 2'); | ||
| }); | ||
|
|
||
| it('passes single severity to API when --status is provided with a single value', async () => { |
There was a problem hiding this comment.
Title mentions "passes single severity" but uses --status, I think it should be changed
| await issuesClient.searchIssues({ | ||
| projects: 'my-project', | ||
| statuses: 'OPEN,REOPENED', | ||
| issueStatuses: 'OPEN,REOPENED', |
There was a problem hiding this comment.
Is REOPENED a valid status? It's not in VALID_STATUSES
| `Invalid status(es): '${options.status}'. Valid statuses are: ${VALID_STATUSES.join(', ')}`, | ||
| ); | ||
| } | ||
| options.status = statuses.join(','); |
There was a problem hiding this comment.
Minor feedback, but it mutates the incoming options parameter, maybe you could assign in a value, as we do below for sev?
| }); | ||
|
|
||
| it('should pass statuses parameter', async () => { | ||
| it('should pass issueStatuses parameter', async () => { |
There was a problem hiding this comment.
Minor feedback, but there's a test to check that --severity throws when invalid, but not for --status. It's already handled in the integration tests, but maybe for having consistent tests?
2 participants
No description provided.