Code review feedback about jdiff HOT 15 CLOSED

Looks like ToleranceType will reject tolerance=0 as invalid - intentional?

@glennmatthews Yeap, that is intentional. A check with tolerance = 0 is an exact_match check type.

from jdiff.

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.

@glennmatthews Exclude filter make sense only when jmspath traverse a verbose json object. In case of list of dicts - tesxtFSM case - exclude filter is not needed. See 'sw_upgrade' tests vs 'raw_novalue_exclude'

Add comment in code.

from jdiff.

Should ToleranceType check that tolerance is between 0 and 100? Or are there valid use cases for a >100% tolerance?

@glennmatthews there might be cases where we want a value bigger that 100% - for example memory or bandwidth upgrade. I will add though a assertion to the min value that should not be lower than 0 as I do not se cases where we want to define negative tollerance

from jdiff.

Thanks for finding the time to look into this. Seems like there is some work to do. I will go through it and let you know once done.

from jdiff.

@pszulczewski any chance you can look at this point?

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.

from jdiff.

@chadell there are few point regarding CI. Is it something you can look at?

from jdiff.

Is there a reason that CI needs to run pylint and pytest in containers, but the other linting can run with INVOKE_LOCAL=True?

No strong reason, simply using Python cookiecutter template.

from jdiff.

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.

@glennmatthews the idea here is to store in mock file only the devices show commands. The rest would be inline as would be mainly few lines for testing code utilities

from jdiff.

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?

@glennmatthews as part of library design discussion with @chadell, we decided to keep the same signature for all the check method available, hence **kwargs. We will point the user to the documentation to find information on which args define whitin **kwargs

from jdiff.

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?

@glennmatthews as part of library design discussion with @chadell, we decided to keep the same signature for all the check method available, hence **kwargs. We will point the user to the documentation to find information on which args define whitin **kwargs

Hmm. I'll check with Christian to understand the reasoning here. Given that different checks need to be called with different kwargs, I don't understand why we wouldn't want to document that explicitly in the method signatures.

from jdiff.

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?
@glennmatthews as part of library design discussion with @chadell, we decided to keep the same signature for all the check method available, hence **kwargs. We will point the user to the documentation to find information on which args define whitin **kwargs

Hmm. I'll check with Christian to understand the reasoning here. Given that different checks need to be called with different kwargs, I don't understand why we wouldn't want to document that explicitly in the method signatures.

The idea behind it was to keep signature consistent across all the checks. Hence, the implementation of validation method for each check. Was quite a long discussion between the 2 possible options :)

from jdiff.

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?
@glennmatthews as part of library design discussion with @chadell, we decided to keep the same signature for all the check method available, hence **kwargs. We will point the user to the documentation to find information on which args define whitin **kwargs

Hmm. I'll check with Christian to understand the reasoning here. Given that different checks need to be called with different kwargs, I don't understand why we wouldn't want to document that explicitly in the method signatures.

The idea behind it was to keep signature consistent across all the checks. Hence, the implementation of validation method for each check. Was quite a long discussion between the 2 possible options :)

Still doesn't make sense to me to have evaluate() have a different signature per CheckType but a completely generic validate() signature, but I'll leave well enough alone then. :-)

from jdiff.

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?
@glennmatthews as part of library design discussion with @chadell, we decided to keep the same signature for all the check method available, hence **kwargs. We will point the user to the documentation to find information on which args define whitin **kwargs

Hmm. I'll check with Christian to understand the reasoning here. Given that different checks need to be called with different kwargs, I don't understand why we wouldn't want to document that explicitly in the method signatures.

The idea behind it was to keep signature consistent across all the checks. Hence, the implementation of validation method for each check. Was quite a long discussion between the 2 possible options :)

Still doesn't make sense to me to have evaluate() have a different signature per CheckType but a completely generic validate() signature, but I'll leave well enough alone then. :-)

@glennmatthews @chadell
Moved validate method into private one as we do not need to expose it. Hope it make more sense

from jdiff.

@glennmatthews I have disabled mypy from tests as there are several error that will required some time to fix. Since is not mandatory to have it, i have raised an issue to fix them while we are going ahead with the OSRB process. https://github.com/networktocode-llc/netcompare/issues/65

Let me know if you have any concern

from jdiff.

@glennmatthews I have disabled mypy from tests as there are several error that will required some time to fix. Since is not mandatory to have it, i have raised an issue to fix them while we are going ahead with the OSRB process. #65

Let me know if you have any concern

UPDATE: @pszulczewski is working on that issue and he should be able to fix all mypy lint errors.

UPDATEUPDATE: Is now done

from jdiff.