Caster icon indicating copy to clipboard operation
Caster copied to clipboard

Add environment variable expansion into path reading

Open kendonB opened this issue 4 years ago • 7 comments

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 avatar Jan 20 '22 05:01 kendonB

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

kendonB avatar Jan 20 '22 05:01 kendonB

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 avatar Jan 20 '22 20:01 kendonB

@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.

kendonB avatar Jan 23 '22 22:01 kendonB

Can you rebase on DropPy2?

LexiconCode avatar Feb 26 '22 06:02 LexiconCode

Can you rebase on DropPy2?

Done now @LexiconCode

kendonB avatar Mar 21 '22 00:03 kendonB

@comodoro are you able to review this PR?

kendonB avatar Aug 02 '22 21:08 kendonB

@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?

comodoro avatar Aug 03 '22 16:08 comodoro