-
Notifications
You must be signed in to change notification settings - Fork 11
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
CFE-3785: Changed cfbs pretty command to always wrap non-empty top level collections #191
Conversation
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.
Great work 🚀
cfbs/pretty.py
Outdated
if not lst and indent == 0: | ||
return "[\n]" | ||
elif not lst: | ||
return "[]" |
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.
@mineralsfree @larsewi How about keeping it on one line if it's completely empty?
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 agree 👍
cfbs/pretty.py
Outdated
if not dct and indent == 0: | ||
return "{\n}" | ||
elif not dct: | ||
return "{}" |
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.
@larsewi @mineralsfree same here
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.
Looks good overall, left one suggestion / question.
A couple of notes:
I edited the PR title to be past tense and perhaps a bit easier to understand. In this repo (same in cf-remote) we release in GitHub, and PR titles get automatically added to changelogs (Changelog: None
has no effect). Also, this change is user facing, so I think it makes sense to include in changelogs.
Ticket: CFE-3785 Changelog: None Signed-off-by: Mikita Pilinka <mikita.pilinka@northern.tech>
Ticket: CFE-3785
Changelog: None
Signed-off-by: Mikita Pilinka mikita.pilinka@northern.tech