-
Notifications
You must be signed in to change notification settings - Fork 221
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
Use _CONFIG_
to separate config prefix and key
#2848
base: main
Are you sure you want to change the base?
Conversation
65afc4b
to
04edbe8
Compare
examples/.cargo/config.toml
Outdated
@@ -33,7 +33,7 @@ PASSWORD = "PASSWORD" | |||
STATIC_IP = "1.1.1.1 " | |||
GATEWAY_IP = "1.1.1.1" | |||
HOST_IP = "1.1.1.1" | |||
ESP_WIFI_CSI_ENABLE = "true" | |||
ESP_WIFI__CSI_ENABLE = "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking at this changed line I wonder if a less subtle prefix would be more user-friendly.
I can imagine users missing the second underscore and having a hard time to figure out why setting cfg-value doesn't have any effect - they won't get any indication of an error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just dropping this in as the "McDonald's idea" of solutions. If anyone knows anything better, let's do that, but until then, this is a fallback :)
We are limited to what we can encode in an environment variable's name, unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure but something like _CFG_
is less likely to be unspotted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree with @bjoernQ, __
is likely to be missed. I think either _CFG_
or personally I'd prefer _CONFIG_
but either is fine with me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also the tedious option of renaming the esp-hal-embassy
crate to esp-embassy
, which would be consistent with literally every other crate (except the procmacros one).
(Unless there's a naming convention I'm missing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Unless there's a naming convention I'm missing)
https://github.com/esp-rs/esp-hal-community?tab=readme-ov-file#contributing-a-crate
09f77b3
to
6a00c53
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
_CONFIG_
to separate config prefix and key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
A lazy attempt to resolve #2846