-
Notifications
You must be signed in to change notification settings - Fork 17
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
chore(iota)!: Set active env when adding new env if empty #5330
base: develop
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 4 Skipped Deployments
|
if let Some(active_env) = &self.active_env { | ||
self.get_env(active_env) | ||
} else { | ||
self.envs.first() | ||
} |
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.
should we also set self.active_env
to self.envs.first()
then? Feels weird that we claim to have an active env but the internal state of the object is different.
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.
It's not a mutable fn and I don't think it should be
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 the API is kind of lying to the user, because the object's internal state doesn't reflect what the method returns.
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 don't think that's true. If there's no env the default is the first, which is represented in the state
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.
currently get_active_env()
and active_env()
(autoimplemented getter) may return different values, and I don't think that's good.
if self.get_env(&env.alias).is_none() { | ||
if self.active_env.is_none() { | ||
self.set_active_env(env.alias.clone()); | ||
} | ||
self.envs.push(env); |
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 think the environment should also be set to active in case the user tried to add the same environment again.
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 not sure I understand. You mean if the env exists and they try to add it again?
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.
yes
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 don't think I agree
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.
okay then ... I would still update the docs for this method to mention that it also sets the new env as active (but only if it was unknown before)
} else { | ||
self.envs.first() | ||
} | ||
pub fn get_env(&self, alias: &str) -> Option<&IotaEnv> { |
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 don't really know how to feel about this change tbh
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.
How so?
Description
Much like #5173, when we call
iota client new-env
we do not set the active env if there wasn't one already. This PR changes that (and does a little additional cleanup).Additionally, this PR skips prompting for an env when you call
iota client new-env
with no config.Before
client.yml
After
client.yml