Skip to content

Improve ug_util.py#1013

Merged
TheRealFalcon merged 2 commits into
canonical:mainfrom
sshedi:misc-fixes
Sep 13, 2021
Merged

Improve ug_util.py#1013
TheRealFalcon merged 2 commits into
canonical:mainfrom
sshedi:misc-fixes

Conversation

@sshedi
Copy link
Copy Markdown
Contributor

@sshedi sshedi commented Sep 8, 2021

Signed-off-by: Shreenidhi Shedi sshedi@vmware.com

Improve ug_util.py

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the refactor. I left two nits inline, but I'm ok merging without them.

Some of these changes definitely make the code clearer (removing unnecessary len() calls, simplying if blocks and loops, etc), but some are also purely cosmetic (comment line length, single vs double quotes, removing unnecessary parens). In general, in the future, I think I'd prefer to only make cosmetic changes if we're modifying the code in question, especially since we're likely to adopt black at some point in the future.

Comment thread cloudinit/distros/ug_util.py Outdated
Comment thread cloudinit/distros/ug_util.py Outdated
Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
@sshedi
Copy link
Copy Markdown
Contributor Author

sshedi commented Sep 10, 2021

Thanks for the refactor. I left two nits inline, but I'm ok merging without them.

Some of these changes definitely make the code clearer (removing unnecessary len() calls, simplying if blocks and loops, etc), but some are also purely cosmetic (comment line length, single vs double quotes, removing unnecessary parens). In general, in the future, I think I'd prefer to only make cosmetic changes if we're modifying the code in question, especially since we're likely to adopt black at some point in the future.

I understand. There were some typos and other mistakes in comments so I started fixing them and ended up making modifications in all places. But your suggestion is valid, I will keep it in mind.

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