introduced option to specify file encoding

[d-chris]

fixes #283

introduced option to specify file encoding when overwriting files.

default values

• ini-file: None
• cli-option: "utf-8"

ini-file always overwrite command line option

command line option

--text-file-encoding=TEXT_FILE_ENCODING
                        Specify encoding for files.

configuration file

  text_file_encoding (string):
                        Default encoding for text files.

[pllim]

Thanks!

More eyes on review would be good. The diff looks reasonable to me.

[pllim]

Should the default here be consistent with addoption default (utf-8)?

[bsipocz]

Yes, it should be consistent, though I'm not sure if we should change the default to utf-8 or leave as is now?

[d-chris]

I decided to set encoding=None to avoid changing the behavior of the write_modified_file function since I am not familiar with your codebase. I wanted to ensure that the function behaves as it did before.

From a developer's perspective, I would prefer using "utf-8" as the default encoding.

[d-chris]

any changes required?

i guess the default value of write_modified_file is uncritical.

default value for cli option defaults to "utf-8" which will be passed on.

[bsipocz]

@pllim - bugfix or enhancement?

[bsipocz]

Thank you! The diff looks good to me. However, this will definitely need a test, the one in the bug report issue would be great. And will eventually need an entry in the changelog, though we need to agree on if this is a feature or a bugfix first.

[pllim]

bugfix or enhancement?

Hmm... It does introduce a new option, so more like enhancement?

[d-chris]

i tried to implement some tests

but seems with with testdir.inline_run(p, "--doctest-plus-generate-diff=diff", "--text-file-encoding", encoding) that the DebugRunnerPlus has an issue.

Traceback (most recent call last):
  File "C:\Python313\Lib\doctest.py", line 1395, in __run
    exec(compile(example.source, filename, "single",
         ~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                 compileflags, True), test.globs)
                 ^^^^^^^^^^^^^^^^^^^
  File "<doctest test_file_encoding_utf8.f[0]>", line 1
    print(☆)
          ^
SyntaxError: invalid character '☆' (U+2606)`

see commit f91f597

[d-chris]

changed PR to Draft

todos

• fix issue with syntax error
• refactore my pytest functions, split into seperate tests for diff and overwrite
• write a small contribution.md / requirements-dev.txt, how i set up .venv to develop for windows

[bsipocz]

@d-chris - the last todo point should definitely not be mixed into this PR

[d-chris]

for testing implemented alternative fixture to generate temporary python files, due issue #285

[d-chris]

somehow a can't trigger action within my fork, any tips?

i created a new action (outside of the PR scope) -> pass https://github.com/d-chris/pytest-doctestplus/actions/runs/14041267921

so the small fix from 09979a3 solved the issue

[d-chris]

fixed in 09979a3

[pllim]

Github Actions has to be enabled explicitly by maintainer if it detects a first time contributor. It is how they attempt to prevent bitcoin miners. Nothing you could have done on your side. I just enabled the CI. Thanks for your patience!

[d-chris]

Thanks for your patience—this is my first GH contribution ever!

Is there anything else I need to do to finish the PR?

[pllim]

Since @bsipocz requested changes, would be nice if she could re-review. 🙏

[d-chris]

any changes required?

i guess the default value of write_modified_file is uncritical.

default value for cli option defaults to "utf-8" which will be passed on.

[bsipocz]

Since @bsipocz requested changes, would be nice if she could re-review

Yeap, I plan to come back to this and the other PR later this week.

[bsipocz]

OK, this looks good to me. My only question is if we really need to add a new config option or reusing the existing doctest_encoding would be sufficient. Have you looked into that option? Obviously it only affects the adding a new parser option and not the actual code fixes where you added the usage of encoding.

[bsipocz]

Oh, and this will need a changelog entry, but I can deal with that at release time.

[bsipocz]

Hmm, failures look relevant for pytest < 7.4, but I haven't looked into the details.

Possible options:

• find an easy workaround that makes this feature work; or only have it for 7.4+
• we can consider dropping support for older versions. We don't really follow SPEC0 for these infrastructure tools but keep support until we run into a blockage, and I would be open to discuss if this is a blockage (7.3 will just about pass the 2 year limit in a couple of days)

[d-chris]

i use option --config-file to specify the ini file while running my tests.

whats confuses me i always run tox before a review, and i always pass all tests

ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --config-file /tmp/pytest-of-runner/pytest-0/test_basic_file_encoding_overwrite3/pytest.ini

i guess there i just have to rewrite my tests to pass the configuration file in the correct way. dropping support should not be really necessary?

[bsipocz]

ahh, indeeed. --config-file was new in 7.4: https://github.com/pytest-dev/pytest/releases/tag/7.4.0

[d-chris]

-c should do the trick

pytest --version & pytest --help | findstr "config"
This is pytest version 5.1.0, imported from D:\temp\pytest5\.venv\lib\site-packages\pytest.py
                        configuration file raise errors.
  -c file               load configuration from `file` instead of trying to
                        locate one of the implicit configuration files.
test session debugging and configuration:
  -h, --help            show help message and configuration info
  --trace-config        trace considerations of conftest.py files.

[bsipocz]

This all looks good now. Thank you @d-chris for your contribution and patience during the review process.