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
Add ability for pre_window commands to parse yaml arrays (as well as strings) #482
Conversation
pre_window
commands to parse yaml arrays (as well as strings)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.
Hey @rpassis! Thanks for your PR! ✨
Overall, this looks good! I just have one suggestion, and I've commented on the relevant line.
@@ -135,7 +135,8 @@ def pre_window | |||
elsif pre_tab? | |||
yaml["pre_tab"] | |||
else | |||
yaml["pre_window"] | |||
pre_window = yaml["pre_window"] | |||
pre_window.is_a?(Array) ? pre_window.join("; ") : pre_window |
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.
Since this will be the third time this logic is used in this file, we should probably break it out into a private method and throw it at the bottom of the class somewhere.
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.
@rpassis Thanks for making that change! I just noticed one more minor issue. I've commented on the appropriate line. Sorry for not catching it before! 😬
@@ -135,17 +131,14 @@ def pre_window | |||
elsif pre_tab? |
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 just noticed that there are several conditional branches in this method. While multiple commands don't make sense for rbenv
or rvm
, would you mind also adding a call to parsed_parameters
here with yaml["pre_tab"]
?
@rpassis Thanks for making those changes! This LGTM 👍 I just wish I knew why Code Climate is so upset, but it won't tell me 🤔 |
Currently, only
pre
commands accept yaml strings and arrays. This commit adds the ability forpre_window
commands to also accept yaml strings and arrays.