Skip to content

chore(cli): loop over options ignored for prepackaged publish - #2335

Merged
netomi merged 2 commits into
eclipse-openvsx:mainfrom
trivikr:cli-ignoring-option-loop
Oct 19, 2026
Merged

netomi merged 2 commits into
eclipse-openvsx:mainfrom
trivikr:cli-ignoring-option-loop

Conversation

@trivikr

@trivikr trivikr commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Replace seven near-identical checks in the publish action with a single loop over [flag, value] pairs.

Warnings and their order are unchanged.

@trivikr
trivikr force-pushed the cli-ignoring-option-loop branch from 1d73280 to 457f051 Compare October 15, 2026 18:42
@netomi

netomi commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

The refactor looks correct and behaviour-neutral: same conditions, same order, and none of the seven options has a default, so each is undefined unless passed, as the old checks assumed. Clearing target unconditionally inside the block is equivalent.

A few points, the first one for this PR:

  1. No test covers the warnings. cli/test/unit/main.spec.ts has no case for the "Ignoring option" warnings, so "warnings and their order are unchanged" isn't checked. A spec that publishes a prepackaged file with all seven flags and asserts the warnings in order would cover it.

The rest is pre-existing and better suited to a follow-up:

  1. The --packagePath route warns about nothing. ovsx publish -i ext.vsix --target linux-x64 --baseContentUrl ... silently drops the target and the other packaging options: doPublish (publish.ts:72-76) turns the .vsix path into extensionFile and deletes target. The same flags with a positional ext.vsix do warn.
  2. The ignore rule lives in two places. These seven options are checked in main.ts, while --pre-release is warned about separately in publish.ts:85. Moving the whole list into doPublish, after extensionFile is resolved, would fix 2 and 3 together: one list, both entry routes, --pre-release included.
  3. Minor: only target is cleared, so the other "ignored" options still reach publish(). Nothing reads them on the prepackaged route today, so it has no effect yet. The flag names are hand-written strings that could drift from the .option(...) declarations if one is renamed.

@trivikr

trivikr commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author
  1. No test covers the warnings

Added in f7482cb

Replace seven near-identical checks in the publish action with a
single loop over [flag, value] pairs. Warnings and their order are
unchanged.

Signed-off-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
Signed-off-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
@trivikr
trivikr force-pushed the cli-ignoring-option-loop branch from f7482cb to eca47b0 Compare October 19, 2026 02:24
@netomi
netomi merged commit 826cba1 into eclipse-openvsx:main Oct 19, 2026
5 checks passed
@trivikr
trivikr deleted the cli-ignoring-option-loop branch October 19, 2026 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants