Skip to content

Conversation

@chadell
Copy link
Contributor

@chadell chadell commented Jan 4, 2022

Summary of changes:

  • Consolidate utils functions in only 3 files, with some clear purpose: data normalisation, jmespath parsing and diff helpers
  • Consolidate code around get_value
  • Expose the CheckType from the package init
  • Added some small comments for clarification

For sure, this is not perfect, but it's a first step

This PR addresses #20 and #21, proposing a simpler way to interact with helper functions needed by CheckType.

@chadell
Copy link
Contributor Author

chadell commented Jan 5, 2022

@lvrfrc87 ping me when you are available and we can talk about it.
My main questions are about the auxiliar methods, and how to make them sustainable

@lvrfrc87
Copy link
Collaborator

Meeting set-up for 11th Jan at 11.30 GMT +1

Copy link
Collaborator

@lvrfrc87 lvrfrc87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few comments/notes

Returns:
Evaluated data, may be anything depending on JMESPath used.
"""
if exclude and isinstance(output, Dict):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: would need to test what is returned if exclude is an empty list or enforce list length

if not any(isinstance(i, list) for i in values): # check for multi-nested lists if not found return here
return values

for element in values: # process elements to check is lists should be flatten
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am not mistaken, this data structure (nested list) is required in order to associate the reference key with each value. FYI: @grelleum

# TODO: Not sure how this is working becasyse from `jmespath.search` it's supposed to get a flat list
# of str or Decimals, not another list...
for item in element:
if isinstance(item, dict): # raise if there is a dict, path must be more specific to extract data
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about if not isinstance(item, list) ?

values = flatten_list(values) # flatten list and rewrite values
break # items are the same, need to check only first to see if this is a nested list

paired_key_value = associate_key_of_my_value(jmspath_value_parser(path), values)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better rename in key_value_association ?


result_item = {}

# TODO: Why the 'value' dict has always ONE single element? we have to explain
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chadell are you saying that user can specify more than one parameter to be evaluated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not? the method signature supports it accepting a "list of"

@chadell chadell changed the title [WIP] Refactor side methods Refactor side methods Jan 13, 2022
@chadell chadell marked this pull request as ready for review January 13, 2022 10:12
@pszulczewski
Copy link
Collaborator

pszulczewski commented Jan 17, 2022

@chadell, @lvrfrc87

To fix the failing tolerance checks, I had to remove:

        if not isinstance(values, List):
            raise ValueError(f"Internal error processing Jmespath result. Got {type(values)} instead of a List")

from get_value method, which should be more generic.
If there is any data constraint it should be part of evaluators which are specific for check types. I see some evaluators have that data check already in place.
For instance tolerance uses recursion, so it doesn't need such constraint.
Also users should be able to use any JMESPath in get_value, because they may want to to compare extracted data with exact_match.

Copy link
Collaborator

@pszulczewski pszulczewski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT, ship it as this is mainly file structure change.
Changes to naming and other refactors can be done with subsequent PRs.

@chadell chadell merged commit f432a52 into main Jan 18, 2022
@chadell chadell deleted the evaluator-runner-refactor-draft branch January 18, 2022 04:19
pszulczewski pushed a commit that referenced this pull request Jan 19, 2022
* Consolidate extract_values_from_output into CheckType.get_value

* Simplify evaluator's file to contain only entrypoints

* Small changes to improve comprehension

* Small code refactor and comments

* rename check_type name and include the main class in package init for easier import

* Consolidate utils functions into less files, with some purpose


Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants