Skip to content

Conversation

@a-detiste
Copy link
Contributor

Hi,

This is a more general version of the Debian patch:

https://salsa.debian.org/python-team/packages/python-maison/-/blob/debian/master/debian/patches/0002-remove-toml.patch?ref_type=heads

We don't have rtoml in Debian (yet?), but we have a lot of old things to remove.

I'm not convinced by the performance argument of rtoml for reading tiny text files.

https://wiki.debian.org/Python/Backports

@dbatten5
Copy link
Owner

dbatten5 commented Sep 3, 2025

Thanks for raising this. The GitHub actions for this repo are a bit messed up at the moment so I need to fix those before I can get round to merging this in

@dbatten5
Copy link
Owner

Hi @a-detiste I finally got round to updating the tooling for this package, would you like to rebase and update your PR?

from functools import lru_cache
from typing import Any

import toml
Copy link
Owner

Choose a reason for hiding this comment

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

You could also remove toml from the dependencies with uv remove toml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried

git/maison$ uv remove toml
error: No `project` table found in: `/home/tchet/git/maison/pyproject.toml`

Copy link
Owner

Choose a reason for hiding this comment

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

That's strange, there's definitely one there. Have you pulled latest main branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to git pull

@dbatten5
Copy link
Owner

There are a few failing CI checks, would you be ok to address them?

@a-detiste
Copy link
Contributor Author

There are a few failing CI checks, would you be ok to address them?

I don't know how to fix this:
"Required test coverage of 100.0% not reached. Total coverage: 98.98%"

37 | with open(self.filepath, "rb") as fd:
| ^^^^
38 | return dict(tomllib.load(fd))
39 | except tomllib.TOMLDecodeError as exc:
|
help: Replace with Path.open()

I find this new syntax distastefull but I can do it

@dbatten5
Copy link
Owner

I don't know how to fix this:
"Required test coverage of 100.0% not reached. Total coverage: 98.98%"

Actually can't see a way to fix that so feel free to amend this line to a 99

try:
return dict(toml.load(self.filepath))
except toml.decoder.TomlDecodeError as exc:
with Path.open(self.filepath, "rb") as fd:
Copy link
Owner

@dbatten5 dbatten5 Sep 25, 2025

Choose a reason for hiding this comment

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

I think the linter was suggesting self.filepath.open(mode="rb") as fd:

@a-detiste
Copy link
Contributor Author

a-detiste commented Sep 25, 2025 via email

@a-detiste
Copy link
Contributor Author

everything should be fine now

Copy link
Owner

Choose a reason for hiding this comment

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

these fixtures aren't being used now so you could remove them all and remove the tomli_w dependency

@dbatten5 dbatten5 merged commit 8c7b842 into dbatten5:main Oct 5, 2025
15 checks passed
@dbatten5
Copy link
Owner

dbatten5 commented Oct 5, 2025

Thanks @a-detiste !

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.

2 participants