plugin: add external Association class to be used in plugin
#412
Conversation
Problem: the bank_info struct does not contain the name of the bank in the struct, which can make it more tedious to find the name of the bank because you have to look at the key of the map in order to find it. It would be more convenient if the name was also in the struct. Add the bank name to the struct attached with each user/bank combination.
cmoussa1
added
new feature
cmoussa1
changed the title
Association class to be used in pluginAssociation class to be used in plugin
| * SPDX-License-Identifier: LGPL-3.0 | ||
| \************************************************************/ | ||
|
|
||
| #include "accounting.hpp" |
There was a problem hiding this comment.
What is accounting.cpp for if all the code is in accounting.hpp? Do you plan to implement methods on the Association class in this file later?
There was a problem hiding this comment.
Yes, I was planning on adding any methods as well as standalone functions that relate to any Accounting objects in this file (I know of at least three at the moment).
src/plugins/mf_priority.cpp
Outdated
| @@ -479,7 +469,7 @@ static int query_cb (flux_plugin_t *p, | |||
| * Unpack a payload from an external bulk update service and place it in the | |||
| * multimap datastructure. | |||
| */ | |||
| static void rec_update_cb (flux_t *h, | |||
| static void rec_update_cb(flux_t *h, | |||
There was a problem hiding this comment.
I've been told before that including stray whitespace changes is bad, but I'll let it slide ;)
There was a problem hiding this comment.
Oops! This was an unintended change. Thanks for catching it; will revert
| const std::string string1 = "hello, world!"; | ||
| const std::string string2 = "hello, world!"; | ||
|
|
||
| ok (string1 == string2, "i can perform a basic string comparison test"); |
There was a problem hiding this comment.
Oh, my bad, I had written that test when I was trying to get the unit test file to actually run with make check. Sorry about that! I'll just remove this test altogether since it's not related to any flux-accounting stuff.
| const std::string& bank) | ||
| { | ||
| users_def_bank[userid] = bank; | ||
| } |
There was a problem hiding this comment.
Is calling this helper function really any easier than writing the line of code it wraps?
There was a problem hiding this comment.
Not really; now that you mention it, it'd probably just make more sense to write the line of code directly instead of creating a function for it. Will fix this 👍
Problem: The plugin uses its own bank_info struct to hold user/bank information and has a number of methods for access and modification of these structs. As the plugin's feature set has grown, so have the requirements for the bank_info struct, resulting in a very large and hard-to-parse piece of code. Begin to clean up this plugin. Start by creating a new Association class and place it in a separate file that gets compiled with the plugin. Replace all instances of "struct bank_info" with the new "Association" class type.
Problem: There is no unit testing framework that exists for the class (and eventually methods and functions) for the Association class. Begin to add some unit tests for this class.
cmoussa1
force-pushed
the
add.association.class
branch
from
f552d6f to
8821c68
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #412 +/- ##
==========================================
+ Coverage 81.00% 81.36% +0.36%
==========================================
Files 20 22 +2
Lines 1437 1465 +28
==========================================
+ Hits 1164 1192 +28
Misses 273 273
|
|
Thanks for reviewing @jameshcorbett! Setting MWP here |
Background
As mentioned in #410, the plugin uses its own
bank_infostruct to hold user/bank information and has a number of methods for access and modification of these structs. As the plugin's feature set has grown, so have the requirements for thebank_infostruct, resulting in a very large and hard-to-parse piece of code. It would benefit the readability of this plugin if some basic features were abstracted out to an external class that the plugin could just be#included to the plugin.This PR begins this process of moving some of the definitions and features related to accessing user/bank information from the plugin's internal map to an external class.
accounting.hppandaccounting.cpp, in this PR, are proposed to now hold the following:Associationclass, an object which holds a set of accounting information of a user who belongs to a bank.References to this new
Associationclass are now used in place of the oldstruct bank_info.I also added a unit testing framework for
accounting.*; for now, it just has a test that ensures direct map access works to successfully fetch anAssociation. As more features are abstracted out of the plugin, this testsuite can grow to ensure things work as expected.Fixes #411