Skip to content
This repository has been archived by the owner on Nov 17, 2022. It is now read-only.

Fix Codelist hashing #247

Merged
merged 8 commits into from
Nov 22, 2017
Merged

Fix Codelist hashing #247

merged 8 commits into from
Nov 22, 2017

Conversation

hayfield
Copy link
Contributor

@hayfield hayfield commented Nov 16, 2017

Codelists contain Codes within a set. This means that they are in no particular order. Hash functions require input in the same order to produce the same output.

This sorts the Codes in a Codelist before hashing so that Codelists with the same Codes hash the same no matter which order the set provides them in.

Until this point, this is merely tested indirectly.
These tests ensure that the equality checks work as expected
Hash functions are dependent on the order of input.
Codes in Codelists are stored in a set. This does not have a fixed order. As such, it is necessary to sort the output before hashing.

This adds tests that Codelists hash correctly, and fixes the hashing.
Codelist hashing has been fixed
@hayfield hayfield added bug-fixes Fixes to some sort of bug. codelists Relating to IATI Codelists. complete A PR that is in a state that is ready for review. tests Relating to tests and testing. incomplete A PR that is in a state that is not ready for review. and removed complete A PR that is in a state that is ready for review. labels Nov 16, 2017
@hayfield
Copy link
Contributor Author

Need to add complete attribute to hash and equality checks.

The attribute was added in #45, though the equality and hash checks weren't updated
@hayfield
Copy link
Contributor Author

Failing test - None cannot be compared with str - python 2/3 difference! :D

A Code with no value isn't very useful.It also confuses use since it cannot be assumed that the value will be a string.
@hayfield hayfield added complete A PR that is in a state that is ready for review. and removed incomplete A PR that is in a state that is not ready for review. labels Nov 16, 2017
@hayfield hayfield requested a review from a team November 16, 2017 13:12
@hayfield hayfield mentioned this pull request Nov 16, 2017
2 tasks
@hayfield hayfield added incomplete A PR that is in a state that is not ready for review. and removed complete A PR that is in a state that is ready for review. labels Nov 16, 2017
@hayfield
Copy link
Contributor Author

Equality needs checking in both directions

The __eq__ function takes self and other, with each being handled differently.
As such, tests need performing in both directions encase there is an implementation that works one way but not the other.
@hayfield hayfield added complete A PR that is in a state that is ready for review. and removed incomplete A PR that is in a state that is not ready for review. labels Nov 16, 2017
Copy link
Contributor

@allthatilk allthatilk left a comment

Choose a reason for hiding this comment

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

Some small docstring things.


@pytest.mark.parametrize('codelist', iati.default.codelists('2.02').values())
def test_codelist_diff_completeness_not_equal(self, codelist, cmp_func_different):
"""Check that two Codelist with the same codes but different completeness are not deemed to be equal."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Codelist -> Codelists


@pytest.mark.parametrize('codelist', iati.default.codelists('2.02').values())
def test_codelist_diff_num_codes_not_equal(self, codelist, cmp_func_different):
"""Check that two Codelist with the same name but different codes are not deemed to be equal."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Codelist -> Codelists


@pytest.mark.parametrize('codelist', iati.default.codelists('2.02').values())
def test_codelist_diff_code_name_not_equal(self, codelist):
"""Check that two Codelist with the same name but a Code with a different name are not deemed to be equal."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Codelist -> Codelists

Copy link
Contributor

Choose a reason for hiding this comment

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

Also needs rephrasing as not clear to read.


@pytest.mark.parametrize('codelist', iati.default.codelists('2.02').values())
def test_codelist_diff_code_name_same_hash(self, codelist):
"""Check that two Codelist with the same name but a Code with a different name have the same hash."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Codelist -> Codelists + rephrasing


@pytest.mark.parametrize('codelist', iati.default.codelists('2.02').values())
def test_codelist_diff_code_value_not_equal(self, codelist, cmp_func_different):
"""Check that two Codelist with the same name but a Code with a different value are not deemed to be equal."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Codelist -> Codelists + rephrasing

@hayfield
Copy link
Contributor Author

@dalepotter @allthatilk Any suggestions on how to rephrase the docstrings?

@hayfield hayfield added incomplete A PR that is in a state that is not ready for review. and removed complete A PR that is in a state that is ready for review. labels Nov 22, 2017
This is in response to a review of #247
@hayfield hayfield requested a review from a team November 22, 2017 11:36
@hayfield hayfield added complete A PR that is in a state that is ready for review. and removed incomplete A PR that is in a state that is not ready for review. labels Nov 22, 2017
@hayfield hayfield dismissed allthatilk’s stale review November 22, 2017 11:36

changes to docstrings made

Copy link
Contributor

@allthatilk allthatilk left a comment

Choose a reason for hiding this comment

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

🐠

@hayfield hayfield merged commit 41d1ef2 into dev Nov 22, 2017
@hayfield hayfield deleted the test-codelist-equality branch November 22, 2017 11:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug-fixes Fixes to some sort of bug. codelists Relating to IATI Codelists. complete A PR that is in a state that is ready for review. tests Relating to tests and testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants