Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add short option for initrd kernel version #3543

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

Conversation

keszybz
Copy link
Member

@keszybz keszybz commented Feb 22, 2025

No description provided.

Copy link
Contributor

@behrmann behrmann left a comment

Choose a reason for hiding this comment

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

I'm -0 to the first commit, since I find it a bit harder to read, but the other two changes are good, just a nit and a fix to CI needed.

Both styles were used by the existing code: ['--foo=bar'] and ['--foo', 'bar'].
Switch to the former exclusively. A single arg like '--foo=bar' is easier to
read and/or select&paste in the log output. Also, we avoid explicit str()
wrappers in a bunch of places.
--kernel-version is the option that I use the most often. Having a short
option would be nice.

'-v' would another option for the option, but I think it could be confused
with --version.
@keszybz keszybz force-pushed the initrd-version-short-option branch from 799907d to 587b6ac Compare February 26, 2025 17:13
@keszybz
Copy link
Member Author

keszybz commented Feb 26, 2025

Updated. I think the first commit makes sense because we already had a mix of both styles and I don't like the inconsistency. I agree that the "compressed" form can be harder to read for some forms of the value argument, but I think this is outweighed by the fact that the resulting invocation command is easier to read (and select&paste if desired). But if there is no agreement on that commit, I can drop it.

Copy link
Contributor

@behrmann behrmann left a comment

Choose a reason for hiding this comment

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

I'm sorry you're seeing so much minor CI fallout. With this change mypy seems to more aggressively want to unify the type of like-named variables. The below renamings fix that.

*(["--package-cache-directory", os.fspath(config.package_cache_dir)] if config.package_cache_dir else []), # noqa: E501
*(["--local-mirror", str(config.local_mirror)] if config.local_mirror else []),
"--incremental", str(config.incremental),
*([f"--compress-output={p}"] if (p := config.compress_output) else []),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*([f"--compress-output={p}"] if (p := config.compress_output) else []),
*([f"--compress-output={c}"] if (c := config.compress_output) else []),

*([f"--prepare-script={os.fspath(script)}" for script in config.tools_tree_prepare_scripts]),
"--output=tools",
*(["--source-date-epoch", str(config.source_date_epoch)] if config.source_date_epoch is not None else []), # noqa: E501
*([f"--source-date-epoch={p}"] if (p := config.source_date_epoch) is not None else []),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*([f"--source-date-epoch={p}"] if (p := config.source_date_epoch) is not None else []),
*([f"--source-date-epoch={e}"] if (e := config.source_date_epoch) is not None else []),

# for free to join this conversation on GitHub. Already have an account? # to comment
Development

Successfully merging this pull request may close these issues.

2 participants