python: move and rename some classes for reusability #4669
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Problem: The job-specific JobInfoFormat class embeds JobFormatter and HeaderFormat classes and a filter_empty() method, all of which are not necessarily job-specific classes and methods. This means that other utilities and code that generate formatted output can't reuse these classes and methods. Move the JobFormatter class to flux.util.UtilFormatter and embed the generic HeaderFormatter class in flux.util.OutputFormat. Also move the useful filter_empty() method to flux.util.OutputFormat where it can be reused. Also move JobDatetime, which is also not really job-specific, to flux.util.UtilDatetime, and the fsd() function from flux.job.info into flux.util.
Problem: mypy doesn't like the lack of type information for the UtilFormat.headings attribute. It complains: .../python/flux/util.py:336: error: Need type annotation for 'headings' Add type annotation for UtilFormat.headings.
Problem: The original provided format is lost after the OutputFormat constructor if the prepend option is used. This means the filter_empty() method returns the prepended format string, which can cause errors where the prepended format fields are not expected. Save the original format string in OutputFormat and add an orig parameter that, when true, returns the original string instead of the modified format string.
Problem: The OutputFormat.filter_empty() method returns a new instance of the class, but this makes it difficult to use this function from subclasses of OutputFormat that have different constructor arguments. Change the filter_empty() method to return the new format string, so that the caller can construct the result with the appropriate constructor.
Problem: The OutputFormat constructor validates that all fields in the format are in the self.headings list, but if the supplied format contains a "0." prefix then the fields definitely won't match and the constructor erroneously raises an exception. Remove any "0." prefix from format fields before checking for validity.
|
Thanks! I'll set MWP. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR moves some code around in
flux.utilandflux.job.infoto enhance reusability (required for some follow up PRs), including some other cleanup.In summary:
JobFormatterandHeaderFormatclass are embedded in theJobInfoFormatclass, but these classes are not job specific. MoveJobFormattertoflux.util.UtilFormatterand embedHeaderFormatin the more generalOutputFormatclass.JobDatetime, which is also not really job-specific, toflux.util.UtilDatetime, and thefsd()function fromflux.job.infointoflux.util.filter_empty()toOutputFormatand modify its return type to be a new format string instance of an instance of theOutputFormatclass for reusability.OutputFormatconstructor when a format string that already has0.prefixes is used.