Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add persistent config (storing dynamic changes in file) #131

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cloud-rocket
Copy link

Please consider this contribution to allow storing dynamic config changes in file.

@sampsyo
Copy link
Member

sampsyo commented May 12, 2021

Hi! This is a good start, but it's also pretty simple. Have you tested this in a larger application? Do you know whether this behaves as expected in multi-layered configurations, for example (as in, it doesn't overwrite one config file with values from other config files)? It would be great to hear more of your thinking behind how this approach will be robust to a wide variety of possible scenarios.

It also occurs to me to mention that, if the intent here isn't to cover lots of use cases but instead to embody a "point solution" for one specific application, perhaps it doesn't need to be part of the library. It would be possible to implement this class in your own application code and use it there, if it's sufficient for a single use case.

@cloud-rocket
Copy link
Author

cloud-rocket commented May 13, 2021

@sampsyo ,

1st of all, thanks for bringing up this package - it makes a lot of sense from different perspectives.

I understand that this approach is simple, even so simple that I kind of expected it to be in place when I started using the package and discovered that all the heavy lifting of manipulating the YAML was already implemented.

Yes, it does solve my existing issue, but I am open to discuss how to improve and generalize the solution, if you want something like this to be eventually merged and it's not going to be a waste of everyone's time.

I'll give some background of my specific case or a case I think should be targeted by this package - an application having a default config (hardcoded) in addition to file config and it also accepts command line config overrides. The application can also be reconfigured remotely (or via GUI) and this dynamic configuration should persist between restarts.

Toward some of your comments:

Do you know whether this behaves as expected in multi-layered configurations

I am dumping the whole config into file. I will check how it works for layered stuff, but do you have any specific concern here? Or maybe I don't understand what do you mean by "multi-layered". Is it about nested config or multiple configs per app?

it doesn't overwrite one config file with values from other config files

It does override the pre-configured config file every time from scratch. Again, I guess I don't understand the question

robust to a wide variety of possible scenarios

I guess I don't have the whole picture in place (except maybe for my case). But can you suggest what scenarios should be targeted?

What do you think?

@sampsyo
Copy link
Member

sampsyo commented May 13, 2021

Thanks for the extra background!

Broadly, I think this seems like useful functionality for cases like the one you've mentioned. But the thing that makes me a little worried is that it's sort of specialized to a simple case: where you only have one configuration file, and the file that needs updating is that configuration file (in the user config directory). It's not clear how elegantly this would deal with beets's -c override option, for instance.

Maybe a good destiny for this code would be to make it a kind of cookbook "recipe" that we put into the docs? Applications that need this can easily incorporate the code, but doing so (as opposed to baking it into the library) will make it 100% clear what it does and what the limitations might be.

@cloud-rocket
Copy link
Author

@sampsyo ,

I am actually using the override with command line to my application and the changes are updated and stored in the config file. The idea is to store any overrides or external configurations and to have them persisted when the application is restarted without specifying any external configuration overrides once again.

This is a separate class - so I don't think it actually interferes with existing functionality.

I will double-check the -c option.

I also planned to add another pull request implementing __delitem__ and remove functions to support removing configuration items.

@sampsyo
Copy link
Member

sampsyo commented May 13, 2021

This is a separate class - so I don't think it actually interferes with existing functionality.

Right, I agree with that. The reason that I think it might be better as a "recipe" rather than provided by the library directly is purely human, not technical: the latter could cause confusion because people might not understand what they're getting themselves into when they use it. Making it a "recipe" in the documentation will help explain to potential users what behavior they should expect exactly.

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.

None yet

2 participants