bubblewrap icon indicating copy to clipboard operation
bubblewrap copied to clipboard

Prettify demo script

Open gurjeet opened this issue 5 years ago • 5 comments

Added spaces to make the trailing line-continuation slashes all line up in the same column. Also moved a few command-line flags around to group together flags of same kind.

Note to reviewers: use command git diff -w to see the changes to exclude the whitespace diff.

gurjeet avatar Nov 24 '20 08:11 gurjeet

Here's the URL to review the code without the whitespace diff https://github.com/containers/bubblewrap/pull/394/files?diff=unified&w=1

gurjeet avatar Nov 24 '20 08:11 gurjeet

Can one of the admins verify this patch? I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once

rh-atomic-bot avatar Nov 24 '20 08:11 rh-atomic-bot

Reviving the old PR.

@alexlarsson , others, please take a look at the minimal diff of rearranging lines; consider ignoring the whitespace diff during review by using the following link.

https://github.com/containers/bubblewrap/pull/394/files?diff=unified&w=1

gurjeet avatar Mar 06 '21 18:03 gurjeet

Added spaces to make the trailing line-continuation slashes all line up in the same column

I personally think this doesn't make it any prettier, and the need to redo the lining-up every time a longer parameter is added is a practical annoyance.

smcv avatar Jun 23 '21 17:06 smcv

group together flags of same kind

This does maybe make sense, although the order of parameters to bwrap does matter - many of the parameters manipulate the filesystem in some way, in sequence - so I think sorting filesystem-affecting parameters by the part of the filesystem they affect (for example, keeping everything that touches /run together) is probably better-practice than sorting them by parameter name.

smcv avatar Jun 23 '21 17:06 smcv