Use declarative summary method for all sub-configs - #4739
svartkanin wants to merge 12 commits into
Conversation
|
Would you like me to review this? Is it ready? |
|
It is ready and I'd appreciate a review |
| 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: |
There was a problem hiding this comment.
Since there is no change to the following, I do not see how SummaryLevel.Detailed is ever used:
-
master:
archinstall/archinstall/lib/global_menu.py
Line 533 in eba1c1a
-
pull request branch:
https://github.com/svartkanin/archinstall/blob/f2be09cf8a5b67d373f9f30a1879073f459d5fef/archinstall/lib/global_menu.py#L533
All the code conditional on level being SummaryLevel.Detailed in summary methods will be dead code.
There was a problem hiding this comment.
This was a forward thinking addition, I added it to be able to use this summary also for the installation confirmation page in follow up changes. That screen currently shows the Json blob which isn't very readable nor understandable so I'm planning to use this new summary for that one too but for that one it'll require the full detailed settings
There was a problem hiding this comment.
Please include details like this in the initial comment of the pull request in the future.
|
|
||
| summary = obj.summary() | ||
| for sub_config in self.sub_cfg().values(): | ||
| summary = sub_config.summary(level) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Explained in the comment above, this is the first use case for the new summary. In a follow up I'll use it for the installation confirmation screen as well which needs all the detailed setting summaries
| } | ||
|
|
||
| @override | ||
| def summary(self, level: SummaryLevel = SummaryLevel.BASIC) -> list[str]: |
There was a problem hiding this comment.
level will need to be changed to _level for some summary methods because of a5e7446.
This branch has not been deployed
3 participants
SubConfigabstract class for all sub-configuration entriessummarymethod which is used to assemble the installation preview