Skip to content
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.

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

Fix comparing _EOF with itself #20

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

pushfoo
Copy link

@pushfoo pushfoo commented Oct 26, 2023

Changes

  1. Make _EOF equal itself
  2. Alter __lt__ and __gt__ to use equality check
  3. Add tests for both old and new _EOF comparison behavior

Code quality tasks:

  • Ran pytest
  • Ran [the flake8 steps specified in .github/workflows/lint-and-test.yml[(https://github.com/pushfoo/python-dahuffman/blob/93d9e7b73e7582b9651ab567a69b3ac1c3cb624e/.github/workflows/lint-and-test.yml#L36-L38)

Copy link
Owner

@soxofaan soxofaan left a comment

Choose a reason for hiding this comment

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

hi,

Many thanks for contributing!

from dahuffman.huffmancodec import _EOF


def test_eq():
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think it's necessary to put these tests in a separate file.
I would just add them in the existing test_dahuffman.py file
Maybe move the tests and fixtures inside a class, e.g. TestEndOfFileSymbol. I'm guessing you used a separate file to keep the fixtures away from existing tests, but you can also achieve that by scoping them to a single class



@pytest.fixture
def to_compare(raw_value, of_type):
Copy link
Owner

Choose a reason for hiding this comment

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

I feel this construct of three fixtures is a bit convoluted and misses some useful things to test against.
I would just make a straight list of values to use, e.g.

[
 b"\0",
 b"abc",
 "abc",
 0, 
 100,
 -1000,
  False,
  True,
  None,
  [],
  (),
  {},
  [-100, 0, 10],
  (-100, 10),
  _EndOfFileSymbol()
]

@pushfoo pushfoo marked this pull request as draft October 28, 2023 04:44
class TestEndOfFileSymbol:

def test_eq(self):
assert _EOF == _EOF
Copy link
Owner

Choose a reason for hiding this comment

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

maybe also test equality _EOF == _EndOfFileSymbol()

[-100, 0, 10],
(-100, 10))
)
def to_compare(request):
Copy link
Owner

Choose a reason for hiding this comment

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

What I meant in a previous comment was pytest allows you to define the fixture as a method of TestEndOfFileSymbol (instead of a module level function), which makes it available only to other methods of that class

assert _EOF < to_compare

def test_gt(self, to_compare):
assert to_compare > _EOF
Copy link
Owner

Choose a reason for hiding this comment

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

I would also assert for !=, and maybe just put everything in same test function to keep it simple:

assert _EOF < to_compare
assert _EOF <= to_compare
assert to_compare > _EOF
assert to_compare >= _EOF
assert to_compare != _EOF
assert not (to_compare == _EOF)

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.

2 participants