Skip to content

Use declarative summary method for all sub-configs - #4739

Open
svartkanin wants to merge 10 commits into
archlinux:masterfrom
svartkanin:refactor-config
Open

svartkanin wants to merge 10 commits into
archlinux:masterfrom
svartkanin:refactor-config

Conversation

@svartkanin

Copy link
Copy Markdown
Collaborator
  • Adds a new SubConfig abstract class for all sub-configuration entries
  • All configs implement the summary method which is used to assemble the installation preview

@svartkanin
svartkanin requested a review from Torxed as a code owner August 30, 2026 10:40
@codefiles

Copy link
Copy Markdown
Contributor

Would you like me to review this? Is it ready?

@svartkanin

Copy link
Copy Markdown
Collaborator Author

It is ready and I'd appreciate a review

Comment thread archinstall/lib/args.py
target.chmod(stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP)

def as_summary(self) -> str:
def as_summary(self, level: SummaryLevel = SummaryLevel.Basic) -> str:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since there is no change to the following, I do not see how SummaryLevel.Detailed is ever used:

All the code conditional on level being SummaryLevel.Detailed in summary methods will be dead code.

Comment thread archinstall/lib/args.py

summary = obj.summary()
for sub_config in self.sub_cfg().values():
summary = sub_config.summary(level)

@codefiles codefiles Sep 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The level default (SummaryLevel.Basic) can be removed from all the summary methods if only as_summary calls them. as_summary already has SummaryLevel.Basic as the default for level and passes it to the summary methods here. If this is unnecessary duplication of the default, make level required for summary methods by removing the default from their parameters.

Comment on lines +14 to +15
Basic = auto()
Detailed = auto()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

UPPER_CASE please.

This branch has not been deployed

No deployments
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