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

manual/options-to-docbook: remove string escaping #34469

Closed
wants to merge 1 commit into from

Conversation

WilliButz
Copy link
Member

@WilliButz WilliButz commented Jan 31, 2018

This removes escaping of strings which lead to a false presentation in the
resulting manpages (e.g. "${someExpression}" in an option's default
value resulted in "\${someExpression}" in the manpages) which cannot be copied as is
and thus might be misleading in a manual.

Multiline-strings are now identified by the contained newlines and escaping them
is not necessary as they already have to be escaped in the module's definition.

-> example of escaped default value

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

This removes escaping of strings which lead to a false presentation in the
resulting manpages (e.g. "${someExpression}" in an option's default
value resulted in "\${someExpression}" in the manpages) which cannot be copied as is
and thus might be misleading in a manual.

Multiline-strings are now identified by the contained newlines and escaping them
is not necessary as they already have to be escaped in the module's definition.

Co-Authored-By: Franz Pletz <fpletz@fnordicwalking.de>
@fpletz
Copy link
Member

fpletz commented Jan 31, 2018

We suspect that the escaping was added because builtins.toXML at some point maybe didn't escape strings correctly?

@edolstra or @nbp: Do you remember why that was added in the first place?

@WilliButz
Copy link
Member Author

I just realized that the backslash in "\${someExpression}" has to be escaped on github, when backticks are not used, in case someone was wondering about the initial comment before :)

@edolstra
Copy link
Member

edolstra commented Feb 5, 2018

This PR seems wrong to me. In general, special characters like \, " and ${ must be escaped when rendering strings. Otherwise a string like "\"" will be rendered as """.

The real issue here is that the contents of defaultText should not be escaped. Unfortunately, it looks like optionAttrSetToDocList' doesn't allow distinguishing between default and defaultText.

@joachifm
Copy link
Contributor

I take it this can be closed?

@Mic92 Mic92 closed this Nov 15, 2018
@WilliButz WilliButz deleted the fix-manual-rendering branch May 14, 2021 10:53
@WilliButz WilliButz restored the fix-manual-rendering branch May 14, 2021 10:53
@WilliButz WilliButz deleted the fix-manual-rendering branch May 14, 2021 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants