You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Is returning a tuple (<dict whose structure depends on the check type>, ,<boolean>) the ideal API for this library? Or would it make sense to define some custom classes/data structures to return, in order make things more user-friendly and more flexible for future expansion?
Consider removing Python 3.6 support as it's past EOL.
Is there a reason that CI needs to run pylint and pytest in containers, but the other linting can run with INVOKE_LOCAL=True?
In pyproject.toml, please disable pylint rules by name rather than by message number, and consider adding comments to explain why rules are disabled - it's not at all obvious for example what rule E5110 might be or why it's disabled.
Looks like tests/unit/ and tests/integration directories are just cookiecutter boilerplate - consider deleting them.
It's not obvious to me why flatten_list flattens [[[[-1, 0], [-1, 0]]]] to [[-1, 0], [-1, 0]] rather than say [-1, 0, -1, 0] - consider adding more details to its docstring to make it clearer what it considers to be a "flattened" list and what it doesn't.
Looks like exclude_filter as implemented accepts either a dict or a list for the data parameter, but the type annotations say it takes a Mapping - perhaps this should be Union[Dict, List] instead?
Personally I find the heavily parameterized tests a bit hard to read and understand (for example, test_diff_generator.py - lots of inputs and expected outputs, very little explanation of why these are the expected outputs for any given set of inputs). Not asking for a refactor/redesign here necessarily, just something to consider. Maybe just adding a few comments to each test case might be sufficient.
For test_diff_generator and test_operators especially, is there a reason that the pre/post data are stored as separate JSON files, while the other test data are defined inline in the test scripts? Might be more consistent and more maintainable to either store it all as JSON or all as code.
test_sw_upgrade_device_state - would renaming test_is_passed to check_should_pass be more accurate?
Is there testing for any of the functions in netcompare.utils.diff_helpers? I see TODOs for a couple of them but I may have overlooked testing for the ones that don't have TODOs either.
Consider adding mypy to the linters in order to validate your type hinting is correct. Not blocking.
The functions in netcompare.utils.jmespath_parsers would benefit from more comprehensive docstrings, IMO.
Is there a reason that CheckType.get_value() only supports/uses exclude when output is a Dict? Given that exclude_filter explicitly can handle both dicts and lists? If this is by design, consider raising an exception in the "else" case rather than silently ignoring the exclude parameter.
Is there a reason tolerance must be an int, rather than say a float?
Should ToleranceType check that tolerance is between 0 and 100? Or are there valid use cases for a >100% tolerance?
For required kwargs, consider explicitly declaring them as such in the validate signature, e.g., for ToleranceType this would be def validate(*, tolerance: int). Also, is there a reason for the concrete implementations of validate to accept wildcard **kwargs at all?
In class Operator, referance_data should be reference_data throughout.
In class Operator, more detailed docstrings would be appreciated.
(<dict whose structure depends on the check type>, ,<boolean>)the ideal API for this library? Or would it make sense to define some custom classes/data structures to return, in order make things more user-friendly and more flexible for future expansion?pylintandpytestin containers, but the other linting can run withINVOKE_LOCAL=True?E5110might be or why it's disabled.tests/unit/andtests/integrationdirectories are just cookiecutter boilerplate - consider deleting them.flatten_listflattens[[[[-1, 0], [-1, 0]]]]to[[-1, 0], [-1, 0]]rather than say[-1, 0, -1, 0]- consider adding more details to its docstring to make it clearer what it considers to be a "flattened" list and what it doesn't.exclude_filteras implemented accepts either a dict or a list for thedataparameter, but the type annotations say it takes aMapping- perhaps this should beUnion[Dict, List]instead?test_diff_generatorandtest_operatorsespecially, is there a reason that the pre/post data are stored as separate JSON files, while the other test data are defined inline in the test scripts? Might be more consistent and more maintainable to either store it all as JSON or all as code.test_sw_upgrade_device_state- would renamingtest_is_passedtocheck_should_passbe more accurate?netcompare.utils.diff_helpers? I see TODOs for a couple of them but I may have overlooked testing for the ones that don't have TODOs either.mypyto the linters in order to validate your type hinting is correct. Not blocking.netcompare.utils.jmespath_parserswould benefit from more comprehensive docstrings, IMO.CheckType.get_value()only supports/usesexcludewhenoutputis a Dict? Given thatexclude_filterexplicitly can handle both dicts and lists? If this is by design, consider raising an exception in the "else" case rather than silently ignoring theexcludeparameter.ToleranceTypewill rejecttolerance=0as invalid - intentional?tolerancemust be an int, rather than say a float?ToleranceTypecheck thattoleranceis between 0 and 100? Or are there valid use cases for a >100% tolerance?validatesignature, e.g., for ToleranceType this would bedef validate(*, tolerance: int). Also, is there a reason for the concrete implementations ofvalidateto accept wildcard**kwargsat all?Operator,referance_datashould bereference_datathroughout.Operator, more detailed docstrings would be appreciated.