Skip to content

Conversation

@laundry-96
Copy link
Contributor

The uiri toml implementation doesn't support some minor features of TOML, such as "Not a homogeneous array".

Given that tomllib is added to Python's stdlib, and supports the above feature, it should be loaded by default, and uiri's toml lib should be loaded as a backup

@laundry-96
Copy link
Contributor Author

@tr11 Is there anything else you'd like me to do to get this merged in?

@tr11
Copy link
Owner

tr11 commented Feb 7, 2024

Looks good, I'll merge it in this week!

@laundry-96
Copy link
Contributor Author

Sorry for the late response, pytest has been fixed by adding some ignores....

Unfortunately, it's the "cleanest" way I can do it for now because we are working w/ two different python versions.
I could do an if/else for python version like how I do it in the toml pytest if you prefer.

TIA

@laundry-96
Copy link
Contributor Author

@tr11
Would you like me to do an if/else wrapped around the imports like my testing code, or is this implementation okay?

@tr11
Copy link
Owner

tr11 commented Feb 16, 2024

Yeah, that seems like a good idea. Better be explicit with what we want to do instead of relying on catching in the except blocks. Thanks!

@laundry-96
Copy link
Contributor Author

laundry-96 commented Feb 17, 2024

@tr11 Agreed! Okay all should be fixed now. Let me know if there are any adjustments you'd like me to make :)

I had to leave the ignore on line 877 due to the different types that come out of the toml.load function, but.

`
____________________________________________________ config/init.py _____________________________________________________

877: error: Argument 1 to "load" has incompatible type "TextIO"; expected "SupportsRead[bytes]" [arg-type]
877: note: Following member(s) of "TextIO" have conflicts:
877: note: Expected:
877: note: def read(self, int = ..., /) -> bytes
877: note: Got:
877: note: def read(self, int = ..., /) -> str
`

The above is the error that 3.11/12 gets, but is non-existent on <= 3.10

I can look into it more in the following week if you'd like

@laundry-96
Copy link
Contributor Author

@tr11 Didn't have azure/aws/gcp dependencies installed so I didn't catch that last issue.
Should be all green now :)

@tr11
Copy link
Owner

tr11 commented Feb 19, 2024

Any thoughts about using tomli instead for older versions? It seems to be a backport of the library so we wouldn't have to maintain a special toml_readtype

@laundry-96
Copy link
Contributor Author

Oh totally! I'll be a wee bit busy this week, but I'll see if I can whip something up by Friday

@tr11
Copy link
Owner

tr11 commented Feb 19, 2024 via email

@tr11 tr11 merged commit 2f28254 into tr11:main Feb 19, 2024
@laundry-96 laundry-96 deleted the patch-2 branch February 21, 2024 02:43
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