Skip to content

fix: preserve a leading BOM when set_key and unset_key rewrite a file - #687

Open
MohammedAlkindi wants to merge 2 commits into
theskumar:mainfrom
MohammedAlkindi:fix/preserve-bom-on-rewrite
Open

fix: preserve a leading BOM when set_key and unset_key rewrite a file#687
MohammedAlkindi wants to merge 2 commits into
theskumar:mainfrom
MohammedAlkindi:fix/preserve-bom-on-rewrite

Conversation

@MohammedAlkindi

Copy link
Copy Markdown

set_key and unset_key drop a leading UTF-8 BOM from files that have one.

#640 made the parser strip a leading BOM so the first variable parses. That stripped BOM never reaches the bindings, so the rewrite path writes the file back without it:

>>> pathlib.Path(".env").write_bytes(b"\xef\xbb\xbfa=x\n")
>>> set_key(".env", "b", "y")
>>> pathlib.Path(".env").read_bytes().startswith(b"\xef\xbb\xbf")
False

rewrite() now carries it across, so both writers keep it. Only files that already had a BOM change behaviour. The check is encoding-safe: a latin-1 file never decodes to \ufeff, and utf-16 handles its own BOM in the codec.

Tests: one for set_key, one for unset_key. Both fail on main on the missing BOM and pass with the fix. Full suite 207 passed / 50 skipped, ruff check and ruff format --check clean.

mypy reports one error here, tests/test_fifo_dotenv.py:15: Module has no attribute "mkfifo". It is pre-existing and unrelated: I confirmed it on a clean checkout of main. It appears because I ran mypy on Windows, where os.mkfifo does not exist, so CI will not see it.

Related: #640, #637

The parser strips a leading UTF-8 BOM so the first variable is read
correctly, but the stripped BOM never reaches the bindings that set_key
and unset_key write back out. Rewriting a file that had one therefore
dropped it silently, changing a part of the file the caller did not ask
to touch.

Carry it across in rewrite(), which both writers share.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant