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

Improve formatting #101

Merged
merged 2 commits into from
Dec 6, 2024
Merged

Improve formatting #101

merged 2 commits into from
Dec 6, 2024

Conversation

nirs
Copy link
Member

@nirs nirs commented Dec 5, 2024

  • Reformat long optins idiomatically
  • Unify whitespace

This makes the table much nicer and easier to read.

Signed-off-by: Nir Soffer <[email protected]>
@nirs nirs changed the title Reintent long options idiomatically Reformat long options idiomatically Dec 5, 2024
@nirs nirs requested review from jandubois and removed request for balajiv113 December 5, 2024 23:24
jandubois
jandubois previously approved these changes Dec 5, 2024
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

Now the enum a little higher up is the only place that still uses 4 spaces indentation; maybe make everything consistent?

@nirs
Copy link
Member Author

nirs commented Dec 6, 2024

Thanks, LGTM

Now the enum a little higher up is the only place that still uses 4 spaces indentation; maybe make everything consistent?

I did not notice, adding another commit.

How about adding a clang-format file to auto format the code?

And change to more common and readable style, like linux kernel style? It is hard to work with 2 space indent, and the code look too dense.

The enum was added with 4 spaces by mistake, since we don't have auto
format or editor config setup. Fix to 2 spaces.

Signed-off-by: Nir Soffer <[email protected]>
@nirs
Copy link
Member Author

nirs commented Dec 6, 2024

@jandubois enum fixed now.

@nirs nirs changed the title Reformat long options idiomatically Improve formatting Dec 6, 2024
{"pidfile", required_argument, NULL, 'p'},
{"help", no_argument, NULL, 'h'},
{"version", no_argument, NULL, 'v'},
{0, 0, 0, 0},
Copy link
Member

Choose a reason for hiding this comment

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

We should use clang-format or something for maintaining the format.

Does clang-format or something support keeping this whitespacing?

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea but even if not having automatic formatting is better.

Copy link
Member Author

Choose a reason for hiding this comment

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

All the default styles (LLVM, GNU, Google, Chromium, Microsoft, Mozilla, WebKit) destroy the nice table. Maybe there is an option to align tables, or to disable formatting of part of a file.

Copy link
Member

Choose a reason for hiding this comment

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

I haven't tested it, but according to Disabling Formatting on a Piece of Code you can disable it for a block like this:

// clang-format off
  const struct option longopts[] = {
    {"socket-group",        required_argument,  NULL,   CLI_OPT_SOCKET_GROUP},
    {"vmnet-mode",          required_argument,  NULL,   CLI_OPT_VMNET_MODE},
    {"vmnet-interface",     required_argument,  NULL,   CLI_OPT_VMNET_INTERFACE},
    {"vmnet-gateway",       required_argument,  NULL,   CLI_OPT_VMNET_GATEWAY},
    {"vmnet-dhcp-end",      required_argument,  NULL,   CLI_OPT_VMNET_DHCP_END},
    {"vmnet-mask",          required_argument,  NULL,   CLI_OPT_VMNET_MASK},
    {"vmnet-interface-id",  required_argument,  NULL,   CLI_OPT_VMNET_INTERFACE_ID},
    {"vmnet-nat66-prefix",  required_argument,  NULL,   CLI_OPT_VMNET_NAT66_PREFIX},
    {"pidfile",             required_argument,  NULL,   'p'},
    {"help",                no_argument,        NULL,   'h'},
    {"version",             no_argument,        NULL,   'v'},
    {0, 0, 0, 0},
  };
// clang-format on

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened #104

@nirs nirs mentioned this pull request Dec 6, 2024
6 tasks
@nirs nirs requested review from AkihiroSuda and jandubois December 6, 2024 15:02
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@jandubois jandubois merged commit 019843b into lima-vm:master Dec 6, 2024
11 checks passed
@jandubois jandubois added this to the v1.2.1 milestone Dec 6, 2024
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