Skip to content
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

Prettify demo script #394

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Prettify demo script #394

wants to merge 2 commits into from

Conversation

gurjeet
Copy link

@gurjeet gurjeet commented Nov 24, 2020

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.

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.
@gurjeet
Copy link
Author

gurjeet commented Nov 24, 2020

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

@rh-atomic-bot
Copy link

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

@gurjeet
Copy link
Author

gurjeet commented Mar 6, 2021

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

@smcv
Copy link
Collaborator

smcv commented Jun 23, 2021

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
Copy link
Collaborator

smcv commented Jun 23, 2021

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.

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.

3 participants