fix: support periods in repository names#10845
Merged
Merged
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
repos?(?:itories)?regex inconfignow only special-cases theurlsubkey; previouslyrepositories.<name>.<anything>would work, whereas now other subkeys (e.g.repositories.foo.verify) will no longer be parsed as a separate sub-property—if this is not intentional, consider making the second capture group generic rather than hard-codingurl. - In the
configcommand, when a specific sub-property is requested (e.g..url) but the repository value is not a dict, the code silently returns the whole value; it may be clearer to raise an error in this case to avoid confusing output when the structure is unexpected. - The
split_keyhelper and the updated interfaces currently requirelist[str]; if you want to keep callsites flexible and easier to compose, consider accepting a more genericSequence[str]orIterable[str]and only materializing a list internally.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `repos?(?:itories)?` regex in `config` now only special-cases the `url` subkey; previously `repositories.<name>.<anything>` would work, whereas now other subkeys (e.g. `repositories.foo.verify`) will no longer be parsed as a separate sub-property—if this is not intentional, consider making the second capture group generic rather than hard-coding `url`.
- In the `config` command, when a specific sub-property is requested (e.g. `.url`) but the repository value is not a dict, the code silently returns the whole value; it may be clearer to raise an error in this case to avoid confusing output when the structure is unexpected.
- The `split_key` helper and the updated interfaces currently require `list[str]`; if you want to keep callsites flexible and easier to compose, consider accepting a more generic `Sequence[str]` or `Iterable[str]` and only materializing a list internally.
## Individual Comments
### Comment 1
<location path="src/poetry/console/commands/config.py" line_range="195-197" />
<code_context>
+ )
- value = repo
+ if m.group(2):
+ # User asked for a specific sub-property (e.g. .url)
+ value = repo.get(m.group(2), repo) if isinstance(repo, dict) else repo
+ else:
+ value = repo
</code_context>
<issue_to_address>
**issue (bug_risk):** Returning the full repo config when the requested sub-key is missing is surprising and may hide configuration issues.
When `repositories.<name>.url` is requested and `url` is missing, this will return the entire `repo` dict instead of failing or returning `None`. That both hides misconfigurations and violates the expectation that a more specific path returns a more specific value. Consider returning `None` or raising when `m.group(2)` is present but the key is absent, e.g.:
```python
if isinstance(repo, dict):
value = repo.get(m.group(2))
else:
value = repo
```
This preserves the current behavior for `repositories.<name>` while making `repositories.<name>.url` predictable.
</issue_to_address>
### Comment 2
<location path="tests/console/commands/test_config.py" line_range="231" />
<code_context>
@pytest.mark.parametrize(
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests around certificates with repository names containing periods
This PR also relaxes the certificates regex (`certificates.(.+).(cert|client-cert)`) and switches those paths to list-based keys. Please add tests that cover `poetry config certificates.my.repo.cert` / `client-cert` (including the unset case) to ensure the CLI behavior matches the new config semantics and that repo names with periods are preserved correctly in certificate configuration.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
The config source methods (get_property, add_property, remove_property) blindly split keys on periods, which breaks repository names containing periods (e.g. "my.repo"). This causes http-basic, pypi-token, repositories, and certificates config entries to be incorrectly nested. The fix adds a split_key() helper and updates the ConfigSource interface to accept either a dotted string or a pre-split list of key segments. All call sites that embed repository names now pass lists to preserve names with periods as single key segments. Closes python-poetry#1328
- Update repo config regex to separate repo name from .url suffix, fixing test_unset_repo_setting failure where foo.url was treated as the repo name instead of foo with url sub-property - Fix conftest Config.get signature to accept str | list[str] matching the updated base Config.get signature - Add type: ignore[index] for tomlkit Item | Container indexing in test_file_config_source tests
The previous test expected `{'url': 'https://bar.com/simple/'}` (a dict)
when querying `repositories.foo.url`. This was an accidental side effect
of the old regex treating `.url` as part of the repo name, causing a
double-nested `url.url` structure in the config source.
With the fix for periods in repository names, the config now correctly
stores `repositories.foo.url = "https://bar.com/simple/"` (flat), so
querying `repositories.foo.url` properly returns just the URL string.
Apply ruff-format auto-fixes for line length and formatting, and replace list concatenation with unpacking (RUF005) in config.py.
6d97905 to
77593a4
Compare
Member
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/poetry/config/config_source.py" line_range="23-32" />
<code_context>
UNSET = object()
+def split_key(key: str | Sequence[str]) -> list[str]:
+ """Split a config key into its component parts.
+
+ If the key is already a list, return it as-is.
+ If the key is a string, split on periods.
+
+ Use a list when the key contains segments with periods
+ (e.g. repository names like "my.repo").
+ """
+ if isinstance(key, str):
+ return key.split(".")
+ return list(key)
</code_context>
<issue_to_address>
**nitpick:** The `split_key` docstring implies it returns a list unchanged, but it actually accepts any sequence and always copies.
The docstring says "If the key is already a list, return it as-is.", but the function accepts `Sequence[str]` and always does `list(key)` for non-strings, so it both supports more than lists and always copies. If copying is intentional, please update the docstring to say it accepts any `Sequence[str]` and always returns a new list of segments.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
radoering
approved these changes
May 1, 2026
This was referenced May 1, 2026
mwalbeck
pushed a commit
to mwalbeck/docker-python-poetry
that referenced
this pull request
May 10, 2026
This PR contains the following updates: | Package | Update | Change | Pending | |---|---|---|---| | [poetry](https://github.com/python-poetry/poetry) ([changelog](https://python-poetry.org/history/)) | minor | `2.3.4` → `2.4.0` | `2.4.1` | --- ### Release Notes <details> <summary>python-poetry/poetry (poetry)</summary> ### [`v2.4.0`](https://github.com/python-poetry/poetry/blob/HEAD/CHANGELOG.md#240---2026-05-03) [Compare Source](python-poetry/poetry@2.3.4...2.4.0) ##### Added - Add `solver.min-release-age` setting to require package releases to be a certain number of days old before they are considered during dependency resolution ([#​10824](python-poetry/poetry#10824)). - Add `solver.min-release-age-exclude` to exclude selected packages from age filtering ([#​10824](python-poetry/poetry#10824)). - Add `solver.min-release-age-exclude-source` to exclude all packages from selected package indexes from age filtering ([#​10824](python-poetry/poetry#10824)). ##### Changed - Raise an error instead of silently ignoring a package name that is not a dependency when it is passed to `poetry update` ([#​10721](python-poetry/poetry#10721)). - Automatically add a trailing slash to legacy repository URLs (used for publishing) if missing ([#​10785](python-poetry/poetry#10785)). - Require `installer>=1.0.0` ([#​10869](python-poetry/poetry#10869)). - Allow `findpython>=0.8` ([#​10874](python-poetry/poetry#10874)). ##### Fixed - Fix an issue where `requires-plugins` fails on Windows if scheme paths are on different drives ([#​10869](python-poetry/poetry#10869)). - Fix an issue where the order of markers in the lock file was not deterministic ([#​10720](python-poetry/poetry#10720)). - Fix an issue where the wrong command was suggested when `poetry self` commands failed due to an outdated lock file ([#​10715](python-poetry/poetry#10715)). - Fix an issue where `poetry env activate` did not work for bash on Windows ([#​10716](python-poetry/poetry#10716)). - Fix an issue where `poetry debug resolve` failed when there was a package with a marker ([#​10807](python-poetry/poetry#10807)). - Fix an issue where the error message about a build backend failure contained garbled `--config-settings` ([#​10804](python-poetry/poetry#10804)). - Fix an issue where a false warning about a circular dependency was printed ([#​10811](python-poetry/poetry#10811)). - Fix an issue where falsy config values were incorrectly treated as not set ([#​10808](python-poetry/poetry#10808)). - Fix an issue where `poetry publish --build` ignored failing builds and uploaded stale artifacts ([#​10802](python-poetry/poetry#10802)). - Fix an issue where `poetry publish` was aborted instead of retrying after package registration ([#​10801](python-poetry/poetry#10801)). - Fix an issue where zip files were not closed after fetching metadata via `lazy-wheel` ([#​10800](python-poetry/poetry#10800)). - Fix an issue where data fetched via `lazy-wheel` was corrupted when part of it had already been cached ([#​10806](python-poetry/poetry#10806)). - Fix an issue where further packages were installed even though installation should be aborted ([#​10742](python-poetry/poetry#10742)). - Fix an issue where installed packages without a `METADATA` file caused an exception on Python 3.15+ ([#​10860](python-poetry/poetry#10860)). - Fix an issue where `http-basic` could not be set for repository names with periods ([#​10845](python-poetry/poetry#10845)). - Fix an issue where calculating the hash of large wheels failed with a memory error ([#​10814](python-poetry/poetry#10814)). ##### Docs - Clarify the precedence of configuration sources ([#​10757](python-poetry/poetry#10757)). - Add a note about the influence of `.gitignore` on `tool.poetry.packages` ([#​10835](python-poetry/poetry#10835)). ##### poetry-core ([`2.4.0`](https://github.com/python-poetry/poetry-core/releases/tag/2.4.0)) - Update vendored `packaging` to `26.2` ([#​936](python-poetry/poetry-core#936)). </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xNzAuMTkiLCJ1cGRhdGVkSW5WZXIiOiI0My4xNzAuMTkiLCJ0YXJnZXRCcmFuY2giOiJtYWluIiwibGFiZWxzIjpbXX0=--> Reviewed-on: https://git.walbeck.it/mwalbeck/docker-python-poetry/pulls/1743 Co-authored-by: renovate-bot <bot@walbeck.it> Co-committed-by: renovate-bot <bot@walbeck.it>
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
Fixes #1328
my.repo) were broken becauseadd_property,get_property, andremove_propertyin config sources blindly split on., treating periods in the name as key separatorssplit_key()helper that accepts either a dotted string or a pre-splitlist[str], and updated theConfigSourceinterface accordinglyhttp-basic,pypi-token,repositories,certificates) to pass pre-split key lists, preserving names with periods as single key segments[^.]+to.+to allow periods in repository names for certificate configTest plan
DictConfigSourceandFileConfigSource:test_dict_config_source_add_property_with_list_keystest_dict_config_source_get_property_with_list_keystest_dict_config_source_remove_property_with_list_keystest_dict_config_source_add_property_with_periods_in_repo_nametest_file_config_source_add_property_with_list_keystest_file_config_source_get_property_with_list_keystest_file_config_source_remove_property_with_list_keyspoetry config repositories.my.repo https://example.com/simple/should create[repositories."my.repo"]instead of[repositories.my.repo]poetry config http-basic.my.repo user passshould store credentials under"my.repo"key