Skip to content

Fix operator checks to follow other check_type logic. #85

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Sep 23, 2022
Merged

Fix operator checks to follow other check_type logic. #85

merged 2 commits into from
Sep 23, 2022

Conversation

pszulczewski
Copy link
Collaborator

@pszulczewski pszulczewski commented Sep 22, 2022 •
edited
Loading

This PR fixes operator check logic and returned results to make them the same as for other check types.
When the check passes then returned data result is empty, only when the check fails data result is returned to indicate why the check failed.
This is the main behavior for other checks and I think operator check should follow the same behavior.

pszulczewski
pszulczewski commented Sep 22, 2022
View reviewed changes
jdiff/check_types.py
@@ -193,7 +193,7 @@ def _validate(params) -> None: # type: ignore[override]
params_key = params.get("params", {}).get("mode")
params_value = params.get("params", {}).get("operator_data")

if not params_key or not params_value:
if not params_key or params_value is None:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was making all-same check to fail here when False was passed as value.

lvrfrc87
lvrfrc87 reviewed Sep 22, 2022
View reviewed changes
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.

I believe we need to review this along with @chadell. This approach is the negative of what implemented today. I understand that the idea behind that is to unify the way the test return the bool result but I believe it would be confusing adopt this strategy.

lvrfrc87
lvrfrc87 previously approved these changes Sep 22, 2022
View reviewed changes
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.

As discussed, this is now good to go! Thanks for helping me to see this from a different point of view @pszulczewski !

@lvrfrc87
Copy link
Collaborator

@pszulczewski would you mind to update the CHANGELOG and relevant bits in poetry?

@lvrfrc87 lvrfrc87 mentioned this pull request Sep 22, 2022
Closed
lvrfrc87
lvrfrc87 approved these changes Sep 23, 2022
View reviewed changes
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.

LGTM

@lvrfrc87 lvrfrc87 merged commit 7d47d7c into develop Sep 23, 2022
@lvrfrc87 lvrfrc87 deleted the fix_operator_checks branch September 23, 2022 08:47
lvrfrc87 added a commit that referenced this pull request Sep 23, 2022
@lvrfrc87 @itdependsnetworks
@pszulczewski @scetron
Release 0.0.2 (#88)
a539dbe 
* Update readme to start with use cases (#84)

* Update readme to start with use cases

* Apply suggestions from code review

Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>

* Update README.md

Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>

* Doc update (#87)

* Fix operator checks to follow other check_type logic. (#85)

* Fix operator checks to follow other check_type logic.

* Add new release 0.0.2

Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>

Co-authored-by: Christian Adell <christian.adell@networktocode.com>
Co-authored-by: Ken Celenza <ken@celenza.org>
Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>
Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>
lvrfrc87 added a commit that referenced this pull request Sep 27, 2022
@lvrfrc87 @itdependsnetworks
@pszulczewski @scetron
Implement ge and le operator type (#89)
afa094d 
* Release 0.0.2  (#88)

* Update readme to start with use cases (#84)

* Update readme to start with use cases

* Apply suggestions from code review

Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>

* Update README.md

Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>

* Doc update (#87)

* Fix operator checks to follow other check_type logic. (#85)

* Fix operator checks to follow other check_type logic.

* Add new release 0.0.2

Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>

Co-authored-by: Christian Adell <christian.adell@networktocode.com>
Co-authored-by: Ken Celenza <ken@celenza.org>
Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>
Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>

* add operator ge and le

* fix validate tests

Co-authored-by: Christian Adell <christian.adell@networktocode.com>
Co-authored-by: Ken Celenza <ken@celenza.org>
Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>
Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>
@lvrfrc87 lvrfrc87 mentioned this pull request Apr 20, 2023
Merged
lvrfrc87 added a commit that referenced this pull request Apr 20, 2023
@lvrfrc87 @itdependsnetworks
@pszulczewski @scetron @dependabot
Ver. 0.0.3 release. (#99)
7a5ae75 
* Update readme to start with use cases (#84)

* Update readme to start with use cases

* Apply suggestions from code review

Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>

* Update README.md

Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>

* Doc update (#87)

* Fix operator checks to follow other check_type logic. (#85)

* Fix operator checks to follow other check_type logic.

* Add new release 0.0.2

Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>

* Implement `ge` and `le` operator type (#89)

* Release 0.0.2  (#88)

* Update readme to start with use cases (#84)

* Update readme to start with use cases

* Apply suggestions from code review

Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>

* Update README.md

Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>

* Doc update (#87)

* Fix operator checks to follow other check_type logic. (#85)

* Fix operator checks to follow other check_type logic.

* Add new release 0.0.2

Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>

Co-authored-by: Christian Adell <christian.adell@networktocode.com>
Co-authored-by: Ken Celenza <ken@celenza.org>
Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>
Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>

* add operator ge and le

* fix validate tests

Co-authored-by: Christian Adell <christian.adell@networktocode.com>
Co-authored-by: Ken Celenza <ken@celenza.org>
Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>
Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>

* Fix parameter_match check, to support non-normalized data (#90)

* Fix parameter_match check, to support non-normalized data

* Add test

* Update tests.

* remove test print

Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>

* Update mypy

* Bugfix to data normalization

* Fix ref_key parsing

* Update docs

* Issue 92 (#98)

* Base-line tests for future refactor.

* Refactor to simplify code.

* Implement issue #92

* Bump certifi from 2022.9.24 to 2022.12.7 (#95)

Bumps [certifi](https://github.com/certifi/python-certifi) from 2022.9.24 to 2022.12.7.
- [Release notes](https://github.com/certifi/python-certifi/releases)
- [Commits](certifi/python-certifi@2022.09.24...2022.12.07)

---
updated-dependencies:
- dependency-name: certifi
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Christian Adell <christian.adell@networktocode.com>
Co-authored-by: Ken Celenza <ken@celenza.org>
Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>
Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
lvrfrc87 added a commit that referenced this pull request May 12, 2023
@pszulczewski @itdependsnetworks
@scetron @lvrfrc87 @dependabot @Kircheneer
Release v0.0.4 (#102)
1879e6d 
* Update readme to start with use cases (#84)

* Update readme to start with use cases

* Apply suggestions from code review

Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>

* Update README.md

Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>

* Doc update (#87)

* Fix operator checks to follow other check_type logic. (#85)

* Fix operator checks to follow other check_type logic.

* Add new release 0.0.2

Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>

* Implement `ge` and `le` operator type (#89)

* Release 0.0.2  (#88)

* Update readme to start with use cases (#84)

* Update readme to start with use cases

* Apply suggestions from code review

Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>

* Update README.md

Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>

* Doc update (#87)

* Fix operator checks to follow other check_type logic. (#85)

* Fix operator checks to follow other check_type logic.

* Add new release 0.0.2

Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>

Co-authored-by: Christian Adell <christian.adell@networktocode.com>
Co-authored-by: Ken Celenza <ken@celenza.org>
Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>
Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>

* add operator ge and le

* fix validate tests

Co-authored-by: Christian Adell <christian.adell@networktocode.com>
Co-authored-by: Ken Celenza <ken@celenza.org>
Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>
Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>

* Fix parameter_match check, to support non-normalized data (#90)

* Fix parameter_match check, to support non-normalized data

* Add test

* Update tests.

* remove test print

Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>

* Update mypy

* Bugfix to data normalization

* Fix ref_key parsing

* Update docs

* Issue 92 (#98)

* Base-line tests for future refactor.

* Refactor to simplify code.

* Implement issue #92

* Bump certifi from 2022.9.24 to 2022.12.7 (#95)

Bumps [certifi](https://github.com/certifi/python-certifi) from 2022.9.24 to 2022.12.7.
- [Release notes](https://github.com/certifi/python-certifi/releases)
- [Commits](certifi/python-certifi@2022.09.24...2022.12.07)

---
updated-dependencies:
- dependency-name: certifi
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update version and changelog

* update poetry lock

* Update changelog

* Issue 94 & 91 (#100)

* Baseline unittests

* Fix issue #94

* Remove test print

* Fix issue #91

* Relax deepdiff dependency.

* Update pyproject.toml

Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>

* Update poetry.lock

* Relase v0.0.4

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Christian Adell <christian.adell@networktocode.com>
Co-authored-by: Ken Celenza <ken@celenza.org>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>
Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>
Co-authored-by: Federico87 <15066806+lvrfrc87@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Network to Code <federico.olivieri@networktocode.com>
Co-authored-by: Leo Kirchner <leo.kirchner98@gmail.com>
Co-authored-by: Leo Kirchner <Kircheneer@users.noreply.github.com>
lvrfrc87 added a commit that referenced this pull request May 12, 2023
@pszulczewski @itdependsnetworks
@scetron @lvrfrc87 @dependabot @Kircheneer
Release v0.0.4 (#106)
4a887ff 
* Update readme to start with use cases (#84)

* Update readme to start with use cases

* Apply suggestions from code review

Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>

* Update README.md

Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>

* Doc update (#87)

* Fix operator checks to follow other check_type logic. (#85)

* Fix operator checks to follow other check_type logic.

* Add new release 0.0.2

Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>

* Implement `ge` and `le` operator type (#89)

* Release 0.0.2  (#88)

* Update readme to start with use cases (#84)

* Update readme to start with use cases

* Apply suggestions from code review

Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>

* Update README.md

Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>

* Doc update (#87)

* Fix operator checks to follow other check_type logic. (#85)

* Fix operator checks to follow other check_type logic.

* Add new release 0.0.2

Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>

Co-authored-by: Christian Adell <christian.adell@networktocode.com>
Co-authored-by: Ken Celenza <ken@celenza.org>
Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>
Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>

* add operator ge and le

* fix validate tests

Co-authored-by: Christian Adell <christian.adell@networktocode.com>
Co-authored-by: Ken Celenza <ken@celenza.org>
Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>
Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>

* Fix parameter_match check, to support non-normalized data (#90)

* Fix parameter_match check, to support non-normalized data

* Add test

* Update tests.

* remove test print

Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>

* Update mypy

* Bugfix to data normalization

* Fix ref_key parsing

* Update docs

* Issue 92 (#98)

* Base-line tests for future refactor.

* Refactor to simplify code.

* Implement issue #92

* Bump certifi from 2022.9.24 to 2022.12.7 (#95)

Bumps [certifi](https://github.com/certifi/python-certifi) from 2022.9.24 to 2022.12.7.
- [Release notes](https://github.com/certifi/python-certifi/releases)
- [Commits](certifi/python-certifi@2022.09.24...2022.12.07)

---
updated-dependencies:
- dependency-name: certifi
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update version and changelog

* update poetry lock

* Update changelog

* Issue 94 & 91 (#100)

* Baseline unittests

* Fix issue #94

* Remove test print

* Fix issue #91

* Relax deepdiff dependency.

* Update pyproject.toml

Co-authored-by: Patryk Szulczewski <patryk@szulczewski.eu>

* Update poetry.lock

* Relase v0.0.4

* Release v0.0.4 (#105)

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Christian Adell <christian.adell@networktocode.com>
Co-authored-by: Ken Celenza <ken@celenza.org>
Co-authored-by: Stephen Corry <stephen.corry@networktocode.com>
Co-authored-by: Patryk Szulczewski <patryk.szulczewski@networktocode.com>
Co-authored-by: Federico87 <15066806+lvrfrc87@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Network to Code <federico.olivieri@networktocode.com>
Co-authored-by: Leo Kirchner <leo.kirchner98@gmail.com>
Co-authored-by: Leo Kirchner <Kircheneer@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lvrfrc87 lvrfrc87 lvrfrc87 approved these changes

@chadell chadell Awaiting requested review from chadell

@scetron scetron Awaiting requested review from scetron scetron is a code owner

@jeffkala jeffkala Awaiting requested review from jeffkala jeffkala is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pszulczewski @lvrfrc87