Refactor environment parsing #534
Conversation
EverlastingBugstopper
changed the base branch from
master
to
avery/environments
src/settings/project/mod.rs
Outdated
| emoji::WARN | ||
| ); | ||
|
|
||
| // TODO: deprecate --release, remove warnings and parsing | ||
| // switch wrangler publish behavior to act the same at top level | ||
| // and environments | ||
| // brace yourself, this is hairy | ||
| let workers_dot_dev = match environment { | ||
| let workers_dot_dev: bool = match environment { |
There was a problem hiding this comment.
Part of me wonders if there is a way to further simplify the logic around the truth table for workers_dot_dev, release, and route. What we have here is totally serviceable--I think, however, this logic could become even more understandable for folks if we reduced the nested matches and focused on easy-to-follow compound conditionals.
For example, we could use logic that could do the following pseudocode:
- Extract a specified environment only once using one match statement.
- If environment does not exist (is
None, so top level configs used):
a. Ifrelease == trueandworkers_dot_dev.is_some(), throw error message stating conflict between using--releaseandworkers_dot_dev.
b. Ifrelease == trueandworkers_dot_dev.is_none(), warn of deprecation and return false (proceed to deploy to route)
c. Ifrelease == falseandself.workers_dot_dev.is_some()and!self.route.is_empty(), bail withwdd_failure
d. Ifrelease == falseandself.workers_dot_dev.is_some()andself.route.is_empty(), returnworkers_dot_devvalue
e. Ifrelease == falseandself.workers_dot_dev.is_none()return true.
There was a problem hiding this comment.
I'll work on this today :)
There was a problem hiding this comment.
Actually, can we leave this so we can merge refactor today? @gabbifish
There was a problem hiding this comment.
Makes sense to me! I'll save the link to this convo and we can revisit it after this is merged.
| if environment.zone_id.is_some() { | ||
| target.zone_id = environment.zone_id.clone(); | ||
| } | ||
| target.kv_namespaces = environment.kv_namespaces.clone(); |
There was a problem hiding this comment.
Do we want to wrap environment.kv_namespaces in the same conditionals as above? I would think kv inheritance would work the same as other toml fields...
There was a problem hiding this comment.
@ashleymichal and I talked about this and said that it doesn't really make sense to inherit kv namespaces because it's an anti-pattern to use multiple namespaces across different environments. I don't want my staging worker to use my production kv namespaces. In this case - if you wanted to use the same namespace for both, you would just have to copy + paste to explicitly use the same namespaces for both environments. Open to discussion about this.
There was a problem hiding this comment.
This makes sense. It could be worth adding this as a comment to clarify why this logic diverges from the rest of the logic preceding it. Other than that, LGTM + I will approve this!
EverlastingBugstopper
force-pushed
the
avery/environments-refactor
branch
from
354aa69
to
24df3eb
Compare
EverlastingBugstopper
force-pushed
the
avery/environments-refactor
branch
from
24df3eb
to
8ef1a5a
Compare
EverlastingBugstopper
changed the title
This PR changes a lot of my bad Rust code into slightly better Rust code. The main goal here is readability.