python: create new BankFormatter subclass, restructure view-bank to use new class
#525
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
cmoussa1
added
improvement
cmoussa1
force-pushed
the
restructure.bank.views
branch
2 times, most recently
from
49aecd2 to
fc18eb6
Compare
cmoussa1
changed the title
BankFormatter subclass, restructure view-bank to use new classBankFormatter subclass, restructure view-bank to use new class
cmoussa1
force-pushed
the
restructure.bank.views
branch
from
fc18eb6 to
3ae8630
Compare
wihobbs
approved these changes
Dec 26, 2024
| if users: | ||
| for user in users: | ||
| hierarchy += ( | ||
| f"{indent} {bank}|{user[0]}|{user[1]}|{user[2]}|{user[3]}\n" |
There was a problem hiding this comment.
Beware the magic numbers here (and in the lines below...)
| @@ -54,7 +54,7 @@ test_expect_success 'view database hierarchy' ' | |||
| ' | |||
|
|
|||
| test_expect_success 'view database hierarchy in a parsable format' ' | |||
| flux account view-bank -P root > small_no_tie_parsable.test && | |||
| flux account view-bank --tree -P root > small_no_tie_parsable.test && | |||
cmoussa1
force-pushed
the
restructure.bank.views
branch
from
3ae8630 to
bbcfb18
Compare
Problem: The AccountingFormatter class has good overarching methods for printing the results of a query to the flux-accounting database, but there is some functionality specific to viewing bank information from the database that could use its own subclass. Add a new subclass called BankFormatter, which contains unique methods for viewing bank/user information in hierarchical and parsable formats with the view-bank command. Remove the helper functions previously defined in bank_subcommands.py in favor of using this new subclass. Add a --fields optional argument to allow the user to customize which columns they want to be returned when looking at a row of information for a bank.
Problem: view-bank does not have many unit tests, especially for calling view-bank with custom outputs or viewing information in either JSON or table format. Add some unit tests.
Problem: The --users option for the view-bank command lists a lot of columns for every user row in the association_table, which crowds the output with potentially unneccessary information when looking at which users belong to the bank being looked at. The format of this option has been changed to: 1) be formatted in tables, and 2) only include a couple of columns per user row Adjust the expected output of the --users optional argument to account for the use of the BankFormatter subclass.
Problem: The --tree option for the view-bank command still lists the default information for the bank being viewed *before* printing the bank hierarchy, which is not necessary when the user passes the --tree option. Now that the view_bank() function has been reworked to use the new BankFormatter subclass, these options are more distinct from one another, and the --tree option no longer includes this info for the single bank at the top of the output when the --tree option is passed. Adjust the expected output in the sharness tests to account for the removal of the default info for the passed-in bank when calling view-bank with the --tree option.
Problem: The default output of the view-bank command is now JSON by default, but one of the sharness tests still looks for the old format before the introduction of the BankFormatter subclass. Change the expected output to match the new format of calling view-bank with no optional arguments.
Problem: The parsable option for viewing bank hierarchies in the flux-accounting database is meant just to be another option for the --tree option, but now the -P option requires --tree to also be passed when calling view-bank. There are multiple tests in the flux-accounting testsuite that call -P without --tree, and thus, these tests fail. Add --tree to the view-bank -P calls in the testsuite.
Problem: The view-bank command does not support the combination of both --tree and --fields, but there is no check for this in the testsuite. Add a test in t1023-flux-account-banks.t that makes sure the appropriate error message is raised when combining both --tree and --fields.
Problem: The AccountingFormatter class has good overarching methods for printing the results of a query to the flux-accounting database, but there is some functionality specific to viewing bank information from the database that could use its own subclass. Add a new subclass called BankFormatter, which contains unique methods for viewing bank/user information in hierarchical and parsable formats with the view-bank command. Remove the helper functions previously defined in bank_subcommands.py in favor of using this new subclass. Add a --fields optional argument to allow the user to customize which columns they want to be returned when looking at a row of information for a bank.
cmoussa1
force-pushed
the
restructure.bank.views
branch
from
bbcfb18 to
bd78e16
Compare
|
Thanks @wihobbs! I've rebased this branch to catch up to |
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.
Problem
The
AccountingFormatterclass has good overarching methods for printing the results of a query to the flux-accounting database, but there is some functionality specific to viewing bank information from the database that could use its own subclass.This PR adds a new subclass called
BankFormatter, which contains unique methods for viewing bank/user information in hierarchical and parsable formats with theview-bankcommand, particularly with the--tree,--users, and-Poptions. A new--fieldsoption is also added to match the customization available in thelist-bankscommand. It removes the helper functions previously defined inbank_subcommands.pyin favor of using this new subclass.I've added some new unit tests for the output of the reworked
view-bankcommand to the testsuite as well as adjusted a number of expected output files throughout the testsuite to account for the use of the newBankFormattersubclass.