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

docs: Change --indent-string's whitespace representation #9904

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

rogersheu
Copy link
Contributor

@rogersheu rogersheu commented Sep 1, 2024

Type of Changes

Type
βœ“ πŸ“œ Docs

Description

It was noticed that when the all-options.rst gets viewed on docs, multiple spaces get compressed into one space. This was occurring both in the help and in the default setting.

To get around this in the help section, I used U+2002, which is an En Space. I'm flexible on which whitespace character to use, provided it remedies any future confusion.

For the default value, I just left it as a regular space, but added quotation marks so the spacing is clear.

Closes #8392

@Pierre-Sassoulas Pierre-Sassoulas added Documentation πŸ“— Skip news πŸ”‡ This change does not require a changelog entry labels Sep 1, 2024
Copy link

codecov bot commented Sep 1, 2024

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 95.80%. Comparing base (123fef3) to head (0e68059).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #9904   +/-   ##
=======================================
  Coverage   95.80%   95.80%           
=======================================
  Files         174      174           
  Lines       18918    18918           
=======================================
  Hits        18124    18124           
  Misses        794      794           
Files with missing lines Coverage Ξ”
pylint/checkers/format.py 96.40% <ΓΈ> (ΓΈ)

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Thank you the pull request, there's no need for a changelog here I applied the skip News label :)

@@ -219,11 +219,11 @@ class FormatChecker(BaseTokenChecker, BaseRawFileChecker):
(
"indent-string",
{
"default": " ",
"default": '" "',
Copy link
Member

Choose a reason for hiding this comment

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

This change thΓ© default value in the code, it's going to cause problem. There's a mechanism in place to modify the default value for the doc but not the code, see enchent dict of available language for the spell checker or default python interpreter. (On mobile, can't link to the exact place Sorry)

Copy link
Contributor Author

@rogersheu rogersheu Sep 1, 2024

Choose a reason for hiding this comment

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

I see. Yeah, I can see how this might cause a problem.

I'm looking at the arguments for enchant. Is this the one you had in mind?

        (
            "spelling-dict",
            {
                "default": "",
                "type": "choice",
                "metavar": "<dict name>",
                "choices": _get_enchant_dict_choices(enchant_dicts),
                "help": _get_enchant_dict_help(enchant_dicts, PYENCHANT_AVAILABLE),
            },
        ),
...

I see what's happening here, with functions being passed into choices/help, but the real issue is that default has to be four spaces, , exactly to make sure the default remains correct. This means that the value has to be different when interpreted for the conversion to reStructuredText.

Does adding such functionality sound like a reasonable path forward for this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Back on a PC πŸ˜„ I meant this:

DYNAMICALLY_DEFINED_OPTIONS: dict[str, dict[str, str]] = {
# Option name, key / values we want to modify
"py-version": {"default": "sys.version_info[:2]"},
"spelling-dict": {
"choices": "Values from 'enchant.Broker().list_dicts()' depending on your local enchant installation",
"help": "Spelling dictionary name. Available dictionaries depends on your local enchant installation",
},
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing me to this @Pierre-Sassoulas !!

Been busy with work lately, but I'm hoping to find a little time to test out this change soon. πŸ˜…

Copy link
Contributor

github-actions bot commented Sep 1, 2024

πŸ€– According to the primer, this change has no effect on the checked open source code. πŸ€–πŸŽ‰

This comment was generated for commit 0e68059

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation πŸ“— Skip news πŸ”‡ This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation has incorrect rendering of spaces for the indent-string option
2 participants