wscott / fplan

Early retirement financial calculator
GNU General Public License v3.0
26 stars 6 forks source link

switch to tomllib #23

Closed hubcity closed 5 months ago

hubcity commented 5 months ago

This change will allow you to parse a file containing

taxrates = [[0, 10.2]]

The tomllib library will read this, while the toml library in python < 3.11 gives an error message

ValueError: Not a homogeneous array
wscott commented 5 months ago

I am not a Python guy, so I spent a while reading about all the mess of toml libraries this morning. My main issue with this change is that tomllib is first introduced in python 3.11 which is rather new. I see you have tomli as a fallback but you require the user to have installed that manually.

I would like this to just work with pip for someone with an older python. The current macos ships with 3.9.x for example.

My suggestion is to use tomli directly and add tomli as a dependency to the pyproject.toml file.

mildebrandt commented 5 months ago

First, I'm on vacation...but thought I should comment on this. I only have my phone, so I can't test anything I'm about to write. :)

My hunch is the dependency section should have the "tomli" package instead of "tomlib". That would ensure people have it before it's imported. Also, since "tomlib" is part of the standard library, it wouldn't need to be in the dependency section...further making me think it was just a typo.

If you don't want "tomli" installed on systems with Python 3.11 or higher, this answer explains the syntax to achieve that: https://stackoverflow.com/a/35614580

I much rather use the system libraries than third party libraries where possible, and fall back when needed.

hubcity commented 5 months ago

Thanks for these comments!

I'm not really a Python guy either. I updated pyproject.toml so that tomli will be installed if the python version is < 3.11. I tested this on my python3.10 system in a python virtual env and it seems to work fine.

cmovic commented 5 months ago

FWIW, I don't see the benefits of adding the complexity to use both tomli and tomlib. Assuming tomli is robust (and my sense is that it is), I'd just go with that and add a note to switch to tomlib someday when 3.11 is far back in the rear-view mirror and we can safely assume everyone is running that version or later.

Along these same lines, should we pin down our library versions using requirements.txt? I come from a dev world where everything was pinned down and library upgrades were carefully tested. I'm new to pyproject.toml and based this suggestion on this stackoverflow post https://stackoverflow.com/questions/74508024/is-requirements-txt-still-needed-when-using-pyproject-toml .

-Vic

On Fri, Jan 26, 2024 at 12:37 PM hubcity @.***> wrote:

Thanks for this comments!

I'm not really a Python guy either. I updated pyproject.toml so that tomli will be installed if the python version is < 3.11. I tested this on my python3.10 system in a python virtual env and it seems to work fine.

— Reply to this email directly, view it on GitHub https://github.com/wscott/fplan/pull/23#issuecomment-1912514898, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIMUR7Q3VD5UUVU6K2SISLYQPZVVAVCNFSM6AAAAABCLFB2NCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJSGUYTIOBZHA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

mildebrandt commented 5 months ago

I don't have a strong opinion about using the third-party library exclusively, but my preference is for how it's coded currently. Either way, you'll be waiting many years before broad deployments of 3.11.

As for pinning dependencies, the requirements.txt file is usually only for developers. It has no effect during a "pip install" of the package. If really needed, those pinned versions should be put in the pyproject.

For apps that are installed by end users, you want to stay as flexible as possible with the dependencies. So, don't pin to exact versions. It is preferable to indicate the lowest version of a dependency that the app will run against. But, for a tool like this, it's fine to not even do that unless using a cutting edge feature of the dependency.

cmovic commented 5 months ago

Thanks for the guidance. I come from an enterprise dev environment where we pinned versions explicitly with “pip -r requirements.txt” installs.

I don’t think the version matters for most of the packages but do wonder if we will want to pin the LP solver. Since I’m sitting in an airport and not in front of a computer I’ll just leave it at “wondering” for now.

Vic

On Friday, January 26, 2024, Chris Mildebrandt @.***> wrote:

I don't have a strong opinion about using the third-party library exclusively, but my preference is for how it's coded currently. Either way, you'll be waiting many years before broad deployments of 3.11.

As for pinning dependencies, the requirements.txt file is usually only for developers. It has no effect during a "pip install" of the package. If really needed, those pinned versions should be put in the pyproject.

For apps that are installed by end users, you want to stay as flexible as possible with the dependencies. So, don't pin to exact versions. It is preferable to indicate the lowest version of a dependency that the app will run against. But, for a tool like this, it's fine to not even do that unless using a cutting edge feature of the dependency.

— Reply to this email directly, view it on GitHub https://github.com/wscott/fplan/pull/23#issuecomment-1912909724, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIMUR3BRS2TIDLUNPW6LLLYQRMY7AVCNFSM6AAAAABCLFB2NCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJSHEYDSNZSGQ . You are receiving this because you commented.Message ID: @.***>