Skip to content

Add include-optional directive. - #102

Open
thet wants to merge 1 commit into
mxstack:mainfrom
thet:thet/include-optional
Open

thet wants to merge 1 commit into
mxstack:mainfrom
thet:thet/include-optional

Conversation

@thet

@thet thet commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor
  • Add include-optional for INI configuration files that may be absent, skipping missing local files and HTTP 404 responses while preserving mandatory includes. This allows projects to define an optional include for client-specific customizations.

  • Fix HTTP error logging during INI inclusion and preserve the original exception.

  • Fix relative INI includes from URLs with a directory path.

What this PR solves

In my coredev buildout I have added a

include =
    ...
    mx-custom.ini

File to add my own customizations but keep the changes in git-managed files minimal.
Still, I need to do a git stash; git pull; git stash pop when syncing with upstream.
We do have something similar in buildout.coredev: optional-extends.

image image

/cc @mauritsvanrees @jensens @rnixx

- Add ``include-optional`` for INI configuration files that may be
  absent, skipping missing local files and HTTP 404 responses while
  preserving mandatory includes. This allows projects to define an
  optional include for client-specific customizations.

- Fix HTTP error logging during INI inclusion and preserve the original
  exception.

- Fix relative INI includes from URLs with a directory path.

Co-authored-by: Codex <noreply@openai.com>
@thet
thet force-pushed the thet/include-optional branch from aca2d40 to a7eb181 Compare September 28, 2026 10:36

@mauritsvanrees mauritsvanrees left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice!
I did not try it, but the code looks good to me.

Tests fail with an error that should be only temporary:

cause: HTTP status server error (503 Service Unavailable) for url (https://pypi.org/simple/hatchling/)

I am rerunning them now.

@mauritsvanrees

Copy link
Copy Markdown
Contributor

Green.

@jensens

jensens commented Oct 10, 2026

Copy link
Copy Markdown
Member

@thet, thanks, useful feature.

I let Claude check out the branch and let it identify a few edge cases against a7eb181.

AI-assisted review follows:

Should be fixed before merge

  1. "Missing" is defined too narrowly.
    • A missing file:///... URL raises URLError (not an HTTPError 404), so loading is aborted, although the README says missing files are ignored.
    • For local paths only FileNotFoundError is caught. NotADirectoryError (a path component is a file) and IsADirectoryError still abort.

Question on semantics

  1. Optional includes cannot override the including file. Optional includes are applied before the including file (same as include), so the main file always wins. If mx.ini sets default-use = false or a version-overrides block, mx-custom.ini cannot change it. It can only add keys that the main file does not set. With buildout's optional-extends, the extending file wins. Is that difference intended? For your use case (avoid git stash when syncing upstream) it might be enough if you only add checkouts, but it should at least be documented.

Smaller things

  1. Double reporting of HTTP errors: logger.error(...) followed by raise reports the same error twice, and the log line does not say which file referenced the URL or whether the include was optional.
  2. Duplicated branches: The two recursive resolve_dependencies(...) calls only differ in the target and http_parent, and directive == "include-optional" is evaluated in both. A single call with a computed target/optional would be easier to maintain.

Not caused by this PR

While testing, we found a few problems in the include resolver that already exist on main: interpolation in include paths, missing cycle detection, and redirects. With include-optional some of them change from a loud error to a silently skipped file, but that is not something to fix here. I opened #103 for them.

Assisted-by: Claude Opus 5.5

This branch has not been deployed

No deployments
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.

3 participants