Build: allow partial override of build steps #11710
Conversation
|
Checks are failing because we have their deps hardcoded They should pass after the PR is merged. |
I think What is the complexity you found while working on this?
I wouldn't start by adding all this complexity to the validation. If users override those steps but they follow what we expect (our contract) that seems fine.
I'm fine start by testing it internally first. We have a few issues from users where we can suggest them to try this approach and receive feedback from them to work in the next iteration. |
There was a problem hiding this comment.
This work looks good to me.
It seems there is no need to apply some of the restrictions implemented in this PR, tho. I left some suggestions that allow using formats together with build.jobs.build overrides.
I also think we should remove the restriction to always override build.jobs.build.html. I didn't see anything blocking us to allow that.
| if not "html" in build_jobs_build: | ||
| raise ConfigError(message_id=ConfigError.HTML_BUILD_STEP_REQUIRED) |
There was a problem hiding this comment.
I think build.jobs.build.html shouldn't be mandatory. That means that users cannot override only any of the other formats like build.jobs.build.pdf or build.jobs.build.epub.
| @@ -692,6 +727,15 @@ def validate_search(self): | |||
|
|
|||
| return search | |||
|
|
|||
| def validate_incompatible_keys(self): | |||
| # `formats` and `build.jobs.build.*` can't be used together. | |||
There was a problem hiding this comment.
I would remove this restriction. It seems pretty common to use the default PDF builder (via formats: [pdf]) , but override build.jobs.build.html. What is the problem this restriction solves?
| build_overridden = config.build.jobs.build is not None | ||
| if build_overridden: | ||
| self.run_build_job("build.html") | ||
| self.run_build_job("build.htmlzip") | ||
| self.run_build_job("build.pdf") | ||
| self.run_build_job("build.epub") | ||
| else: | ||
| self.build_html() | ||
| self.build_htmlzip() | ||
| self.build_pdf() | ||
| self.build_epub() |
There was a problem hiding this comment.
Here is where we can remove the formats restriction:
for format in ["html", "htmlzip", "pdf", "epub"]:
commands = get_dotted_attribute(self.data.config, f"build.jobs.build.{format}", [])
if commands:
self.run_build_job(f"build.{format}")
else:
method = getattr(self, f"build_{format}")
method()That allows users to override only the format they want to override while keeping the default commands when using formats as well.
|
Supporting formats: ["pdf"]
build:
# pdf is duplicated twice
pdf: echo pdf
# This is invalid, as epub isn't in formats.
epub: echo epubIt just feels more explicit having everything in one place, and not depend on other keys. Also, as mentioned in the other issue, overriding one step may require to update all build steps, which will make it not so common to re-use the default commands. |
I don't see a problem in the example you shared and I don't think the formats: ["epub"]
build:
pdf:
- echo pdfIs also valid and will use the default command for ePUB and a custom one for PDF. I don't think we need to perform any kind of validation. |
|
We agreed to move forward with:
|
I was replacing some of the config models with dataclasses, but I found myself re-implementing some helpers that pydantic provides, so I'm introducing that new dep, it has everything we need, we may be able to use it to simplify our validation once all models are migrated to pydantic.
About incompatible options
I decided to just not allow formats when
build.jobs.buildis used, seems just easier that way. But not sure if users may want to just override the html build step while still using the default build pdf step, if that's the case, we may need to support using formats... or have another way of saying "use the default".build.jobs.create_environment is kind of incompatible with python/conda.
Since users will need to manually create the environment,
but they may still want to use the defaults from build.jobs.install,
or maybe they can't even use the defaults, as they are really tied to the default create_environment step.
Which bring us to the next point,
if build.jobs.create_environment is overridden,
users will likely need to override build.jobs.install and build.jobs.build as well...
Maybe just save the user some time and require them to override all of them?
Or maybe just let them figure it out by themselves with the errors?
We could gather more information once we see some real usage of this...
Override them all
I chose to make build.html required if users override that step, seems logical to do so.
Do we want to allow users to build all formats? I'm starting with html and pdf, that seem the most used. But shouldn't be a problem to support all of them. I guess my only question would be about naming,I just went ahead and allowed all, with the same name as formats.htmlziphas always been a weird name, maybe just zip?Docs
I didn't add docs yet because there is the question... should we just expose this to users? Or maybe just test it internally for now?
Closes #11551