Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
kmaphoenix
left a comment
kmaphoenix
left a comment
There was a problem hiding this comment.
Mostly style edits and name space refactors to make it clear how each of the methods should be used.
A couple of areas that could be refactored to allow for faster execution as well.
|
|
||
| return params_list | ||
|
|
||
| def get_param_presets_df(self, flow_name_list=None): |
There was a problem hiding this comment.
@SeanScripts @skyyeat this method doesn't have any input args that would produce an API call to an Agent.
Is this supposed to be an internal private method used by something else?
When I searched the file, I don't see this in use anywhere else.
I'm not sure what the purpose of the method is here.
I would suggest one of the following actions:
- remove the method
- refactor so that it requires an arg that a user can provide so that it will be actionable by the API
| """Gets a dataframe of parameter presets. | ||
|
|
||
| Args: | ||
| flow_name_list (optoinal): list of names of flows to search. |
| param_dfs.append(df) | ||
| #Combine Lists to DataFrame | ||
| if len(param_dfs) > 0: | ||
| return pd.concat(param_dfs).reset_index(drop=True) |
There was a problem hiding this comment.
The nested return followed by return None could be refactored much more cleanly.
After you check the params_dfs length, if it passes, set the results of pd.concat... to a variable and then simply return that variable. this eliminates the need for 2 return statements.
for flow id in flow_id_list:
....
for page in self.page_data:
.....
if len(param_dfs) > 0:
final_df = pd.concat(....)
return final_df| return pd.concat(param_dfs).reset_index(drop=True) | ||
| return None | ||
|
|
||
| def process_param_presets_in_page(self, flow_name, page): |
There was a problem hiding this comment.
@SeanScripts @skyyeat it looks like this should be a private internal function utilized by get_param_presets_df.
Update the semantics of the name to be _process_param_presets_in_page() and then move the method up above the instance methods so it's not confused as something that the user should try to call first hand.
Reminder that the style for method sorting is as follows for this library:
- Class
- Static
- Private
- Instance
Ex:
@classmethod
def my_class_method(cls):
....
@staticmethod
def my_static_method():
...
def _some_private_method(self):
...
def my_instance_method(self):
self._some_private_method()
self.my_static_method()This provides a nice layout for each class so that users can understand which methods they should interact with first hand and which methods are used internally by other methods.
| return pd.concat(df_list) | ||
| return None | ||
|
|
||
| def process_param_presets_in_form(self, flow_name, page_name, page_param): |
There was a problem hiding this comment.
Same here, set to private _blahblah
| return pd.concat(df_list) | ||
| return None | ||
|
|
||
| def process_param_presets_in_route_group(self, |
| return pd.concat(df_list) | ||
| return None | ||
|
|
||
| def get_param_presets_helper(self, |
| "param_value": parameter_preset_values | ||
| }) | ||
|
|
||
| def convert_protobuf(self, obj): |
There was a problem hiding this comment.
@SeanScripts @skyyeat this code is redundant and can be removed.
There is already a set of methods in scrapi_base.ScrapiBase that handle recursion of MapComposite and RepeatedComposite types.
See:
If for some reason these don't fit your exact use case, please take one of the following actions:
- add a new method to scrapi_base to be used by all classes
- modify one of the existing methods in scrapi_base appropriately
If you choose the latter, ensure that you've tested the other classes thoroughly, as there could be many, many dependencies on these methods throughout.
| from proto.marshal.collections.maps import MapComposite | ||
| from proto.marshal.collections.repeated import RepeatedComposite | ||
|
|
||
| # Type aliases |
There was a problem hiding this comment.
@SeanScripts @skyyeat I don't think these global type aliases are necessary.
- DFCXFlow is not used anywhere in this class
- The other 3 are only used 1 time, so there's no real savings or reusability here
|
|
||
| def get_all_page_data(self): | ||
| page_data = {} | ||
| for flow_id in self.flows_map.values(): |
There was a problem hiding this comment.
I think you're unnecessarily looping through the self.flows_map twice across these two methods:
get_all_page_dataget_all_route_group_data
Since you have to loop through the flow to get to both the Page and Route Group data, you should build both sets of data simultaneously as you go through self.flows_map.
Since you are already storing the data in self.page_data and self.route_group_data you can easily update these from within a single method.
3 participants
In the SearchUtil class, adds a function get_param_presets_df to get a dataframe of all parameter presets in the agent, with optional filtering to a list of flows. This covers a common search request, to look for all places where a particular parameter is set, and what value it is set to.
Thanks for your work on this content @skyyeat !