refactor(parser): share plugin-option parser between create and config#10
Merged
Merged
Conversation
CreateCommand previously had its own parse_plugin_option_value with divergent rules (Float coercion of "1.10" → 1.1, no quote handling, no bracket arrays), so inputs that worked under config set silently corrupted under create. Move the working ConfigCommand parser into a new PluginOptionParser module and route both modules through it; the broken local copy on the create side is deleted. Behaviour for config set is unchanged (same code, new home). Also relocate the parse_plugin_option_value contract testset from test_config_command.jl into the dedicated test_plugin_option_parser.jl so it lives next to the function it guards. The CreateCommand-local testset that asserted Float coercion is removed: that contract was the bug. Closes #9
Add three E2E testsets that drive real argv strings through the
shared PluginOptionParser into CreateCommand.execute(dry-run):
quoted-comma values stay single strings (Git.name="Doe, Jane"),
bracket arrays become Vector{String} (Git.ignore=[...]), and
version-like values stay strings (ProjectFile.version=1.10) instead
of being coerced to Float and losing the trailing zero. These were
the exact inputs that silently corrupted under create before the
parser unification.
There was a problem hiding this comment.
Pull request overview
Refactors plugin-option parsing so both create and config set share a single, quote/bracket-aware KEY=VALUE bundle parser, eliminating previously divergent coercion rules (notably the create float-coercion bug for version-like values).
Changes:
- Added
PluginOptionParsermodule withparse_value,split_string, andparse_kv_string. - Updated
CreateCommandandConfigCommandto route plugin option parsing throughPluginOptionParser. - Reorganized and expanded tests: moved parser contract tests into a dedicated file and added
createE2E coverage for the issue #9 acceptance cases.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/plugin_option_parser.jl | Introduces the shared parsing implementation used by both commands. |
| src/create_command.jl | Removes the local value parser and uses PluginOptionParser.parse_kv_string for plugin bundles. |
| src/config_command.jl | Drops local helpers and uses PluginOptionParser.parse_kv_string when applying config set plugin options. |
| src/PkgTemplatesCommandLineInterface.jl | Ensures the new module is included before consumers. |
| test/test_plugin_option_parser.jl | New centralized contract tests for parsing behavior. |
| test/test_create_command.jl | Removes tests for the deleted, broken local parser; adds E2E acceptance tests for unified parsing. |
| test/test_config_command.jl | Removes the old parser contract tests that moved to the new shared test file. |
| test/runtests.jl | Adds the new parser test file to the test suite. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
split_string previously only treated a literal space as a separator, so pasted multiline bundles or tab-separated input failed to split. Use isspace(c) so the implementation matches the "whitespace" docstring and covers tabs and newlines. parse_kv_string previously inserted entries under an empty-string key when given malformed tokens like "=value" or " =x". Skip those tokens so downstream plugin construction and TOML writes never see a blank option name. Both fixes get explicit contract tests.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
split_string previously entered quote mode on any '/" outside an existing quote/bracket context, so name=O'Connor email=x would open a quoted block at the apostrophe, never close it, and swallow email=x into the name value (silently dropping the email option). Apostrophes in real names are common enough that this corrupted realistic input. Add an at-token-start guard so a quote character only opens a quoted block when it immediately follows whitespace, =, or the start of the input. This preserves every canonical case (name="Doe, Jane", a='1 2', leading quoted tokens) while making mid-value quote characters literal. The contract is locked down at both the split_string and parse_kv_string layers.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The previous assertions only checked that ".DS_Store" and ".vscode" appeared in stdout, which would still pass if `ignore` were silently flattened into the single string ".DS_Store,.vscode". Assert on the array formatting (`ignore = [`, `"\\".DS_Store\\""`, `"\\".vscode\\""`) so this test catches the corruption mode it was written to guard against.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The previous docstring said whitespace around keys and values is stripped, which was misleading: split_string splits on whitespace first, so 'k= v' and 'k =v' produce separate tokens that don't form a single option. Spell out that 'key=value' must remain one token and that whitespace around '=' will not be coalesced.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ultimatile
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #9. Both
CreateCommandandConfigCommandaccept the sameKEY=VALUEplugin-option bundle shape, but they previously held independent parsers with divergent rules —CreateCommand's copy did Float-coerce1.10to1.1, ignored quotes, and didn't handle bracket arrays. Inputs that worked underconfig setsilently corrupted undercreate.This PR extracts the working
ConfigCommandparser into a newPluginOptionParsermodule that both commands consume.Changes
refactor(parser)— new modulePluginOptionParser(src/plugin_option_parser.jl) hostingparse_value,split_string,parse_kv_string.ConfigCommand's helpers (parse_plugin_option_value,_split_plugin_option_string,_parse_plugin_kv_string) move into it;ConfigCommandandCreateCommandimport from it. The brokenCreateCommand-localparse_plugin_option_value(Float coercion, no quote handling) is deleted along with the testset that asserted its broken contract. Theparse_plugin_option_valuecontract testset relocates fromtest_config_command.jlinto the newtest_plugin_option_parser.jl.test(create)— three E2E acceptance tests drive realcreateargv strings through the unified parser to the dry-run plan: quoted-comma values stay single strings (Git.name="Doe, Jane"), bracket arrays becomeVector{String}(Git.ignore=[...]), and version-like values stayString(ProjectFile.version=1.10).Behaviour change
For
create: plugin option values now follow theconfig setparser semantics. Decimals stayString, quoted strings preserve internal commas, bracket arrays round-trip. Theconfig setpath is unchanged (same code, new home).Test plan
--git 'name="Doe, Jane" email=x'→Git.name == "Doe, Jane"end-to-end--git 'ignore=[.DS_Store, .vscode]'→ Vector reaches plugin options--projectfile version=1.10→ version stays"1.10", no Float coercionparse_value,split_string,parse_kv_stringcovered with the 12 edge cases audited during researchConfigCommandbehaviour unchanged (existing config tests still pass against the new module)