Conversation
added init py and binop module
FeatureA_6 core functions
FeatureA_6 Basic Arithmetic Functions
Add input validation for binary strings
Update binary.py by deepjit
Revert "Update binary.py by deepjit"
Revert "Add input validation for binary strings"
Added execption.py for input validation.
feature: add binary complement operations
calculator.py now has function drivers for evaluating binary operations.
Feature a 6 tests
Binary Operation Integration with Base Calculator Operation .py file
fixed duplicate divide.
fixed import path for exceptions module
added imports for multiply and divide
Removed project structure section from README.
fixed import path for exceptions module
fixed subtraction function
fixed ones complement function: issue: modular arithmetic kills the carry fix: let the carry naturally extend the bit length
There was a problem hiding this comment.
Pull request overview
Adds a binary-number feature set to the existing calculator codebase, including conversion utilities, binary arithmetic/complement operations, custom exceptions, and CI coverage for the new module.
Changes:
- Introduces
modules/binary.py(+ re-exports) andmodules/exceptions.pyfor binary operations and domain-specific errors. - Extends
Calculatorwith anevaluate()string router to invoke binary features and arithmetic via simple expression parsing. - Adds unit tests for the binary module and updates CI workflow to run tests and report coverage.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
calculator.py |
Adds imports for binary features and implements Calculator.evaluate() expression routing/parsing. |
modules/binary.py |
Implements binary↔decimal conversion, binary arithmetic, and 1’s/2’s complement helpers. |
modules/exceptions.py |
Adds custom exceptions for invalid binary input and negative subtraction results. |
modules/__init__.py |
Re-exports binary functions at the package level. |
tests/testbinary.py |
Adds unit tests covering conversions, arithmetic, complements, and invalid input. |
tests/__init__.py |
Exposes tests via star import (package init). |
test_calculator.py |
Renames duplicate divide tests to distinct names. |
README.md |
Documents binary features, input format, and how to run tests. |
.github/workflows/tests.yml |
Adds GitHub Actions workflow to run tests and produce a coverage report. |
.gitignore |
Adds common Python/IDE/OS ignores. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Add 1 to the one's complement. | ||
| e.g. twos_complement("B'0111") -> "B'1001" | ||
| """ | ||
| raw = _strip(b) | ||
| _validate(raw) | ||
| ones = ones_complement(f"B'{raw}")[2:] | ||
| val = int(ones, 2) + 1 # no modulo | ||
| result = bin(val)[2:].zfill(len(ones)) # zfill won't truncate carry |
There was a problem hiding this comment.
twos_complement() can increase the bit-length when a carry occurs (e.g. input B'0000 returns B'10000). This is inconsistent with ones_complement() (which preserves width) and with typical two's-complement semantics (usually modulo 2^n). Consider masking/truncating to the original width (or explicitly documenting that overflow expands the width) so callers get predictable output.
| Add 1 to the one's complement. | |
| e.g. twos_complement("B'0111") -> "B'1001" | |
| """ | |
| raw = _strip(b) | |
| _validate(raw) | |
| ones = ones_complement(f"B'{raw}")[2:] | |
| val = int(ones, 2) + 1 # no modulo | |
| result = bin(val)[2:].zfill(len(ones)) # zfill won't truncate carry | |
| Add 1 to the one's complement (modulo 2^n, preserving bit-width). | |
| e.g. twos_complement("B'0111") -> "B'1001" | |
| """ | |
| raw = _strip(b) | |
| _validate(raw) | |
| bit_len = len(raw) | |
| ones = ones_complement(b)[2:] | |
| val = (int(ones, 2) + 1) & ((1 << bit_len) - 1) | |
| result = format(val, f"0{bit_len}b") |
|
|
||
|
|
||
| def test_twos_complement_zero(self): | ||
| self.assertEqual(twos_complement("B'0000"), "B'10000") |
There was a problem hiding this comment.
This test codifies twos_complement("B'0000") == "B'10000", which implies the implementation expands width on overflow. If the intent is fixed-width two's complement (mod 2^n), the expected value should stay the same width (e.g. B'0000). Please clarify the intended semantics and align the test with that definition.
| self.assertEqual(twos_complement("B'0000"), "B'10000") | |
| self.assertEqual(twos_complement("B'0000"), "B'0000") |
| @@ -0,0 +1 @@ | |||
| from .testbinary import * No newline at end of file | |||
There was a problem hiding this comment.
tests/__init__.py re-exports tests via from .testbinary import *, which can introduce import side effects and can confuse test discovery/duplicate collection in some runners. If you don't need tests to act as a public package, prefer an empty __init__.py (or remove it entirely) and let pytest discover test modules directly.
| from .testbinary import * |
| def add(self, a, b): return a + b | ||
| def subtract(self, a, b): return a - b | ||
| def multiply(self, a, b): return a * b |
There was a problem hiding this comment.
These one-line method definitions reduce readability and make future edits (e.g., adding type hints/docstrings/logging) harder. Consider expanding them to the standard multi-line form to keep the class consistent with typical Python formatting.
| def add(self, a, b): return a + b | |
| def subtract(self, a, b): return a - b | |
| def multiply(self, a, b): return a * b | |
| def add(self, a, b): | |
| return a + b | |
| def subtract(self, a, b): | |
| return a - b | |
| def multiply(self, a, b): | |
| return a * b |
| val = re.search(r"\((.+)\)", expr).group(1).strip() | ||
| return decimal_to_binary(f"D'{val}" if not val.upper().startswith("D'") else val) | ||
|
|
||
| if re.match(r"^dec\(", expr, re.I): | ||
| val = re.search(r"\((.+)\)", expr).group(1).strip() | ||
| return binary_to_decimal(val) | ||
|
|
||
| if re.match(r"^1s\(", expr, re.I): | ||
| val = re.search(r"\((.+)\)", expr).group(1).strip() | ||
| return ones_complement(val) | ||
|
|
||
| if re.match(r"^2s\(", expr, re.I): | ||
| val = re.search(r"\((.+)\)", expr).group(1).strip() |
There was a problem hiding this comment.
evaluate() uses re.search(...).group(1) without checking the search matched. Inputs like bin() / bin(10 (missing closing paren) will raise AttributeError instead of a controlled ValueError. Consider switching to a single re.fullmatch() (or validating the search result) and raising a user-facing parse error when the expression is malformed.
| val = re.search(r"\((.+)\)", expr).group(1).strip() | |
| return decimal_to_binary(f"D'{val}" if not val.upper().startswith("D'") else val) | |
| if re.match(r"^dec\(", expr, re.I): | |
| val = re.search(r"\((.+)\)", expr).group(1).strip() | |
| return binary_to_decimal(val) | |
| if re.match(r"^1s\(", expr, re.I): | |
| val = re.search(r"\((.+)\)", expr).group(1).strip() | |
| return ones_complement(val) | |
| if re.match(r"^2s\(", expr, re.I): | |
| val = re.search(r"\((.+)\)", expr).group(1).strip() | |
| m = re.search(r"\((.+)\)", expr) | |
| if not m: | |
| raise ValueError(f"Malformed expression: {expression}") | |
| val = m.group(1).strip() | |
| return decimal_to_binary(f"D'{val}" if not val.upper().startswith("D'") else val) | |
| if re.match(r"^dec\(", expr, re.I): | |
| m = re.search(r"\((.+)\)", expr) | |
| if not m: | |
| raise ValueError(f"Malformed expression: {expression}") | |
| val = m.group(1).strip() | |
| return binary_to_decimal(val) | |
| if re.match(r"^1s\(", expr, re.I): | |
| m = re.search(r"\((.+)\)", expr) | |
| if not m: | |
| raise ValueError(f"Malformed expression: {expression}") | |
| val = m.group(1).strip() | |
| return ones_complement(val) | |
| if re.match(r"^2s\(", expr, re.I): | |
| m = re.search(r"\((.+)\)", expr) | |
| if not m: | |
| raise ValueError(f"Malformed expression: {expression}") | |
| val = m.group(1).strip() |
| m = re.match( | ||
| r"(B'[01]+)\s*([\+\-\*\/])\s*(B'[01]+)", expr, re.I | ||
| ) | ||
| if m: | ||
| a, op, b = m.group(1), m.group(2), m.group(3) | ||
| ops = {'+': binary_add, '-': binary_subtract, | ||
| '*': binary_multiply, '/': binary_divide} | ||
| return ops[op](a, b) |
There was a problem hiding this comment.
The binary arithmetic regex matches only a prefix of the expression (no ^...$/fullmatch), so trailing junk like B'1 + B'1 foo would be accepted and partially evaluated. Anchor the pattern to the full string (or use re.fullmatch) so invalid expressions are rejected consistently.
| def evaluate(self, expression: str) -> str: | ||
| """ | ||
| Route string expressions to the correct module. | ||
| Examples: | ||
| "bin(10)" -> "B'1010" | ||
| "dec(B'1010)" -> "D'10" | ||
| "1s(B'1010)" -> "B'0101" | ||
| "2s(B'0111)" -> "B'1001" | ||
| "B'011 + B'010" -> "B'101" | ||
| """ | ||
| expr = expression.strip() | ||
|
|
||
| if re.match(r"^bin\(", expr, re.I): | ||
| val = re.search(r"\((.+)\)", expr).group(1).strip() | ||
| return decimal_to_binary(f"D'{val}" if not val.upper().startswith("D'") else val) | ||
|
|
||
| if re.match(r"^dec\(", expr, re.I): | ||
| val = re.search(r"\((.+)\)", expr).group(1).strip() | ||
| return binary_to_decimal(val) | ||
|
|
||
| if re.match(r"^1s\(", expr, re.I): | ||
| val = re.search(r"\((.+)\)", expr).group(1).strip() | ||
| return ones_complement(val) | ||
|
|
||
| if re.match(r"^2s\(", expr, re.I): | ||
| val = re.search(r"\((.+)\)", expr).group(1).strip() | ||
| return twos_complement(val) | ||
|
|
||
| # Binary arithmetic: "B'011 + B'010" | ||
| m = re.match( | ||
| r"(B'[01]+)\s*([\+\-\*\/])\s*(B'[01]+)", expr, re.I | ||
| ) | ||
| if m: | ||
| a, op, b = m.group(1), m.group(2), m.group(3) | ||
| ops = {'+': binary_add, '-': binary_subtract, | ||
| '*': binary_multiply, '/': binary_divide} | ||
| return ops[op](a, b) | ||
|
|
||
| raise ValueError(f"Unrecognised expression: {expression}") No newline at end of file |
There was a problem hiding this comment.
evaluate() is new public behavior but there are no tests exercising its supported expression forms (bin/dec/1s/2s/arithmetic) or its error handling for invalid expressions. Add unit tests (likely alongside test_calculator.py) to lock in the routing/parsing behavior and prevent regressions.
Integration of Binary Operations to the Base Calculator.