[WIP] python: add AccountingFormatter class, restructure list-banks to use new class
#520
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
force-pushed
the
add.formatter.class
branch
from
e2ca7ab
to
bf71c25
Compare
Problem: flux-accounting has lots of different tables that can be viewed with view-* and list-* commands, but each one has its own function that could probably be cleaned up with some common utility. Begin to clean up this code by creating a new AccountingFormatter class. Take a SQLite Cursor object (the object responsible for executing a SQL query) as part of initialization. Define some formatting methods for this class, such as as_table and as_json(), which will take the Cursor object and format its result in table format or JSON format, respectively, as well as some basic metadata methods, like get_column_names() or get_rows().
Problem: the flux-accounting Python bindings have lots of separate (and probably duplicate) helper code for validating and interacting with the SQLite database. It would be cleaner and easier to maintain if it was centralized in one file that could be referenced throughout the bindings. Add a new SQL utility file to the Python bindings that will have helper functions for interacting with the SQLite database. Add a function to this file that will validate a custom list of table column names against a valid list of table column names.
Problem: The default fields for the bank_table are defined in the list_banks() function, but they will have more than one use, so it makes sense to define these globally in the accounting Python package. Create a definition for the names of the columns in the bank_table so that they can be referenced throughout the Python bindings for flux-accounting.
Problem: The list-banks command defines its own formatting for the output of the command, but the Python bindings now has its own common formatter and SQL utilities. Edit the list-banks command to make use of the new formatter and external Python utilities. Change the default for the --fields optional argument to be None if it is not specified.
Problem: flux-accounting does not have tests that use the common formatter in the Python bindings. Add some sharness tests for the list-bank command with multiple customization options for the different fields and getting output as both a table and as JSON. Add some unit tests for construction of AccountingFormatter objects and ensuring the correct amount of data is returned from executing a query.
cmoussa1
force-pushed
the
add.formatter.class
branch
from
bf71c25
to
edd543b
Compare
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
flux-accounting has lots of different tables that can be viewed with
view-*andlist-*commands, but each one has its own function that could probably be cleaned up with some common utility. Better yet, if the formatting options for all of these commands was centralized, it might make for easier changes in the future and/or customization of output by users.This PR begins to centralize the formatting for flux-accounting by adding a new
AccountingFormatterclass to the Python bindings. The class takes a SQLiteCursorobject that has the results of running a query on the flux-accounting database and offers two methods of formatting its data: data in an adjustable table or data in JSON format. Since this class just takes aCursorobject, this means it can support customization of what fields are formatted and returned (because the query itself can just be customized to not fetch every field).As an example of using this class, I've restructured the
list-bankscommand. It now makes use of this class and, depending on a new optional argument that allows users to specify if they want data in JSON format, will just pass the result of executing the query instead of defining its own format within the function.I've added some basic unit tests for the
AccountingFormatterclass as well as some sharness tests to call thelist-bankscommand with different customization options and returning data in each format.