Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for configuration options #200

Open
mgeisler opened this issue May 28, 2024 · 8 comments
Open

Add support for configuration options #200

mgeisler opened this issue May 28, 2024 · 8 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@mgeisler
Copy link
Collaborator

For Comprehensive Rust, we use a granularity: 0 setting when extracting messages with mdbook-xgettext. This avoids churn in the line numbers.

However, it happens regularly (google/comprehensive-rust#2100, google/comprehensive-rust#1991, google/comprehensive-rust#1950, ...) that people forget to use this setting.

I would love to have a way of specifying the right settings in a configuration file. Something which can turn

MDBOOK_OUTPUT='{"xgettext": {"pot-file": "messages.pot", "granularity": 0}}' \
  mdbook build -d po

into just

mdbook build -d po

or similar.

We could already store the options in our book.toml file today. The reason we don't do this is that this would enable the xgettext output unconditionally for everybody.

Some ideas for solving this:

  • Make mdbook-xgettext required and enabled in every execution. This would be a little wasteful, but it might not matter if it's quick enough? Measure this to get some data.
  • Make mdbook-xgettext optional with optional: true in book.toml. Make it exit early when it sees that it's optional. If this works well, we could use it for the mdbook-pandoc output too. See Pandoc failure during local rendering comprehensive-rust#1911 for a recent example of it failing.
  • Write a small wrapper and ask people to run that instead of calling mdbook-xgettext directly using the above config hack.
  • Other ideas?
@mgeisler mgeisler added enhancement New feature or request good first issue Good for newcomers labels May 28, 2024
@mgeisler
Copy link
Collaborator Author

mgeisler commented May 28, 2024

@max-heller, what do you think of taking advantage of optional: true like this?

I'm basically suggesting that "heavy" output formats like mdbook-pandoc and mdbook-xgettext could change the semantics of this setting to mean: I won't run if I'm optional.

People would then have to override it back to false with a command like this:

MDBOOK_OUTPUT__FOO__OPTIONAL=false mdbook build

This would mean that mdbook build works with both when mdbook-foo is not installed or when the mdbook-foo dependencies are not installed since mdbook-foo will exit immediately. Only if you ask very clearly for mdbook-foo to do something, will it attempt to execute.

We could even make this more clear by letting the output formats do this if another config value is set:

[output.foo]
optional = true
run-when-optional = false

It would all be convention, but it feels like it could be a useful convention for rarely used output formats like the ones discussed here?

@max-heller
Copy link

max-heller commented May 28, 2024

I'm basically suggesting that "heavy" output formats like mdbook-pandoc and mdbook-xgettext could change the semantics of this setting to mean: I won't run if I'm optional.

People would then have to override it back to false with a command like this:

MDBOOK_OUTPUT__FOO__OPTIONAL=false mdbook build

While these semantics (and an associated way to run a specific renderer) would make sense to me if mdbook were to adopt them, I don't think it makes sense to override them to conflict with the standard semantics.

We could even make this more clear by letting the output formats do this if another config value is set:

[output.foo]
optional = true
run-when-optional = false

This makes more sense to me because it is opt-in and preserves the optional semantics in the default case.

Some other thoughts:

  • Renderers could interpret optional as "make a best-effort attempt" and exit "successfully" with a warning when they otherwise would've exited unsuccessfully. This preserves the happy case semantics but avoids breaking the build when the renderer can't complete successfully for some reason. Maybe this is something mdbook itself could adopt backwards-compatibly?

  • It seems like there are two cases we'd like to support:

    1. Don't run this by default because it takes a long time, results in verbose output, ... (mdbook-xgettext from what I can tell). This is better served by "don't run at all when optional" semantics
    2. Don't run this by default (or don't require this to run successfully by default) because it has external dependencies, may not complete successfully without some other preparation (mdbook-pandoc). This may be better served by "don't break the build when optional" semantics

    It'd be nice to come up with a scheme that could accomplish both. I think interpreting optional as "don't break the build" and using run-if-optional to prevent running altogether might do the trick?

@mgeisler
Copy link
Collaborator Author

  • It'd be nice to come up with a scheme that could accomplish both. I think interpreting optional as "don't break the build" and using run-if-optional to prevent running altogether might do the trick?

Yeah, using two options together should work nicely. What I like is that I can hard code everything in the book.toml file and get nice behavior out of the box for people who haven't installed all dependencies.

@max-heller
Copy link

Bikeshed: how about skip-if-optional = true instead of run-if-optional = false?

@mgeisler
Copy link
Collaborator Author

Bikeshed: how about skip-if-optional = true instead of run-if-optional = false?

I'm happy with skip-if-optional!

@max-heller
Copy link

Thinking about this some more, it feels odd to have optional and skip-if-optional (logically, A and A => B) instead of optional and another option that disables the renderer (disabled?) independently of optional (logically, A and B)

@max-heller
Copy link

Thinking about this some more, it feels odd to have optional and skip-if-optional (logically, A and A => B) instead of optional and another option that disables the renderer (disabled?) independently of optional (logically, A and B)

@mgeisler any opinions on this?

@mgeisler
Copy link
Collaborator Author

Thinking about this some more, it feels odd to have optional and skip-if-optional (logically, A and A => B) instead of optional and another option that disables the renderer (disabled?) independently of optional (logically, A and B)

@mgeisler any opinions on this?

Sorry for the delay! Yeah, you're right: having a simple disabled setting would make more sense! So hard-coding optional = true and disabled = true would be useful for the situation where one wants to

  1. Not require people to install the plugin (optional = true makes mdbook happy)
  2. Include the plugin configuration in a config file, but not actually run the plugin by default (disabled = true handles this)

I'll be happy to implement this for mdbook-xgettext as well then.

max-heller added a commit to max-heller/mdbook-pandoc that referenced this issue Jul 10, 2024
… available (#93)

Adds a `disabled` flag to disable rendering even if `mdbook-pandoc` is
available. Since rendering may rely on external dependencies, this can
be used to e.g. disable rendering except in CI where needed dependencies
are known to be installed.

Related to
google/mdbook-i18n-helpers#200 (comment)
mgeisler pushed a commit to google/comprehensive-rust that referenced this issue Jul 18, 2024
)

Implements the approach summarized in
google/mdbook-i18n-helpers#200 (comment)
to disable running `mdbook-pandoc` even if it is available to avoid
dependency-related issues during local rendering.

Should address #1911
since `mdbook-pandoc` will no longer run locally.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants