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

🎨 Refactor core config #214

Merged
merged 24 commits into from
Dec 10, 2023
Merged

Conversation

JMicheli
Copy link
Collaborator

@JMicheli JMicheli commented Dec 9, 2023

As detailed in Issue #191, Stump currently relies on storing configuration variables in the environment to maintain global state in the application. This is undesirable because 1) it means functions' inputs are not the only thing that determines their behavior, 2) configuration is scattered throughout the repo, and 3) configuration doesn't benefit from Rust's robust validation ecosystem.

To address this, this pull request applies fairly comprehensive changes to the stump_core crate as well as the server app (to maintain compatibility with the changes to the core). All values that were configured with environment variables are still configured with the same environment variables, however, those values are now stored in a StumpConfig struct at startup and this struct is maintained in an Arc throughout Stump's runtime.

Other than this, the application should behave exactly the same.

Copy link
Collaborator

@aaronleopold aaronleopold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a few comments/nitpicks, but otherwise this looks great! Thank you so much for taking the time to work on this overhaul

core/src/config/mod.rs Outdated Show resolved Hide resolved
core/src/config/stump_config.rs Show resolved Hide resolved
core/src/config/stump_config.rs Show resolved Hide resolved
core/src/config/stump_config.rs Show resolved Hide resolved
core/src/filesystem/content_type.rs Outdated Show resolved Hide resolved
crates/cli/src/config.rs Outdated Show resolved Hide resolved
core/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@aaronleopold aaronleopold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making those changes - I think this is good to go! Thanks again 👍

@aaronleopold aaronleopold changed the title Resolve Restructure config 🎨 Refactor core config Dec 10, 2023
@aaronleopold aaronleopold merged commit 58b25d9 into stumpapp:develop Dec 10, 2023
6 of 7 checks passed
@aaronleopold aaronleopold mentioned this pull request Feb 18, 2024
@JMicheli JMicheli deleted the config-refactor branch June 16, 2024 08:19
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