kendonB commented 2 years ago

Add environment variable expansion into path reading

Description

Changing a few instances where paths from settings.toml get read without expanding environment variables.

Related Issue

None.

Motivation and Context

This allows users to reference environment variables in settings.toml for some paths. I wanted to use this to share a settings folder between two machines with different file systems via dropbox.

How Has This Been Tested

Ran this on my Ubuntu system and it started up fine.

Types of changes

[x] New feature (non-breaking change which adds functionality)

Checklist

[x] I have read the CONTRIBUTING document.
[x] My code follows the code style of this project.
[x] I have checked that my code does not duplicate functionality elsewhere in Caster.
[x] My code implements all the features I wish to merge in this pull request.
[ ] My change requires a change to the documentation.
[ ] I have added tests to cover my changes.
[x] All new and existing tests pass.

Maintainer/Reviewer Checklist

[ ] Basic functionality has been tested and works as claimed.
[ ] New documentation is clear and complete.
[ ] Code is clear and readable.

kendonB commented 2 years ago

This seems to break user rules - @LexiconCode can you think of why that might be?

kendonB commented 2 years ago

Ok looks like my original approach wasn't right - simpler just to edit settings.settings as it gets read in. Importantly, after it rewrites any new defaults onto disk.

EDIT: Sorry it's still overwriting settings.toml on disk. will figure it out

kendonB commented 2 years ago

@LexiconCode I believe this one is almost ready.

I have a couple of comments in the code.

Recommend squash and merge since the commits go back and forth.

The current approach reads in all paths from settings.toml expanding env variables. Then, whenever it saves, it rereads the paths from disk without expanding. This is fine because caster never overwrites paths from within caster.

The thing I'm not sure of is if we ever want to overwrite paths with what is in memory (the current approach); probably not because that would cause it to overwrite expanded environment variables which would be a bug. Any future overwrite code should be bespoke, read in from disk, edit, then write. So I recommend we just get the code to always read paths from disk before saving using save_config.

LexiconCode commented 2 years ago

Can you rebase on DropPy2?

kendonB commented 2 years ago

Can you rebase on DropPy2?

Done now @LexiconCode

kendonB commented 2 years ago

@comodoro are you able to review this PR?

comodoro commented 2 years ago

@comodoro are you able to review this PR?

I am not sure, because I don't know the purpose of the code. Why can env variable expansion not happen only once loading settings?

dictation-toolbox / Caster