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

♻️ Implement a config macro to simplify stump_config.rs #397

Merged
merged 11 commits into from
Aug 19, 2024

Conversation

JMicheli
Copy link
Collaborator

Although I like the way that StumpConfig works (taking a file, layering the environment on that, etc.), I think it was pretty cumbersome to update, requiring a lot of little edits throughout the file to add a single variable. There's no reason for that to be the case given Rust's macro system.

I wrote a procedural macro that takes an annotated struct as input and generates something functionally identical to the current contents of stump_config.rs, but many lines shorter. Better still, adding new variables should only take a few lines added to the struct definition. I'm pretty pleased with this one.

It passes all of the tests, which I suspect could be simplified as well.

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.

To the best of my ability, I think this looks good! I am very unfamiliar with macros. Thanks for adding this!!

I'm going ahead and approving this, but I likely won't include it as part of 0.0.5. I've already cut that release branch and plan to push it out today (was going to do it yesterday but then that GitHub outage happened). If I have to delay the release until the weekend, I'll include it though 🙂

core/src/config/stump_config.rs Show resolved Hide resolved
crates/stump-config-gen/src/config_vars.rs Show resolved Hide resolved
@aaronleopold aaronleopold changed the title Implement a config macro to simplify stump_config.rs ♻️ Implement a config macro to simplify stump_config.rs Aug 15, 2024
@JMicheli
Copy link
Collaborator Author

Don't delay the release, I'd like to do a bit more work on this and mainly put the pull request up for review purposes.

I'd like to add better error handling to the macro so that it properly highlights the cause of errors instead of panicking and highlighting the entire input.

I also noticed that it causes something to happen with the verbosity setting that differs from before this change - it turns off all logging (which results from verbosity = 0, the default) but prior to this change verbosity on dev instances of the server would be set to 1. Not sure what the interaction is there and I need to investigate a bit before we merge to avoid nightly users having empty logs.

@aaronleopold
Copy link
Collaborator

Don't delay the release, I'd like to do a bit more work on this and mainly put the pull request up for review purposes.

I mostly meant if something came up that made me have to delay 😅 but no problem! I'll re-request myself for review and mark it as draft until you're ready for me to take another look. Hopefully by then I'll have had the time to get a little more acquainted with macros.

If you need anything during your investigation give me a shout!

@aaronleopold aaronleopold self-requested a review August 15, 2024 20:36
@aaronleopold aaronleopold marked this pull request as draft August 15, 2024 20:36
@JMicheli JMicheli marked this pull request as ready for review August 19, 2024 01:17
@JMicheli
Copy link
Collaborator Author

I've made a few more additions, namely adding some documentation to the macro and adding a set of integration tests that fill the role of several tests on stump_config.rs, allowing them to be removed. That file has been significantly reduced in complexity now.

Assuming everything looks good, this should be ready to merge.

core/src/config/stump_config.rs Outdated Show resolved Hide resolved
core/src/config/stump_config.rs Outdated Show resolved Hide resolved
@aaronleopold aaronleopold enabled auto-merge (squash) August 19, 2024 14:35
@aaronleopold aaronleopold merged commit 3a480f3 into develop Aug 19, 2024
5 of 6 checks passed
@JMicheli JMicheli deleted the config-macro branch August 19, 2024 14:56
@aaronleopold aaronleopold mentioned this pull request Sep 6, 2024
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