pullitt#4
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements solutions for a Python programming assessment covering string parsing, algorithmic logic, and test-driven development. The implementation adds functionality for processing flight ticket information, determining leap years, and monitoring reactor status, along with corresponding unit tests.
Key changes:
- Implemented five functions handling ticket parsing, baggage allowance, flight validation, leap year calculation, and reactor status monitoring
- Created a new test file
test_reactor.pyfor reactor status testing - Modified an existing test assertion for leap year validation
- Added VS Code test configuration
Reviewed changes
Copilot reviewed 4 out of 7 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| system_check.py | Implemented all five TODO functions with logic for flight ticket parsing, leap year determination, and reactor status monitoring |
| test_system_check.py | Modified leap year test to incorrectly expect 1900 as a leap year |
| test_reactor.py | New test file for reactor status function with comprehensive test cases |
| .vscode/settings.json | Added VS Code unittest configuration for test discovery |
| pycache/*.pyc | Generated Python bytecode files (not reviewed) |
Comments suppressed due to low confidence (5)
system_check.py:23
- Print statement may execute during import.
print(get_departure_airport("FL-JO234-JNB-CPT-2023"))
system_check.py:50
- Print statement may execute during import.
print(check_baggage_allowance("EC-JO234-JNB-CPT"))
system_check.py:84
- Print statement may execute during import.
print(validate_flight_number("FL-JOABC-JNB-CPT"))
system_check.py:113
- Print statement may execute during import.
print(is_leap_year(2000))
system_check.py:158
- Print statement may execute during import.
print(reactor_status(1500, 200))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return "First Class - 60kg" | ||
| else: | ||
| return "Standard - 0kg" | ||
| print(check_baggage_allowance("EC-JO234-JNB-CPT")) |
There was a problem hiding this comment.
Debug print statement should be removed. This print call appears to be leftover from testing/debugging and should not be present in production code.
| print(check_baggage_allowance("EC-JO234-JNB-CPT")) |
| if b == "JOABC": | ||
| return "Invalid Flight" | ||
| if b.startswith("JO"): | ||
| if int(b[2::]) % 2 == 0: | ||
| return "Valid - Northbound" | ||
| elif int(b[2::]) % 2 != 0: | ||
| return "Valid - Southbound" | ||
| else: | ||
| return "Invalid Flight" | ||
| elif b.startswith("A"): | ||
| if int(b[1::]) % 2 == 0: | ||
| return "Valid - Northbound" | ||
| elif int(b[1::]) % 2 != 0: | ||
| return "Valid - Southbound" | ||
| else: | ||
| return "Invalid Flight" | ||
|
|
There was a problem hiding this comment.
Hardcoded special case check for "JOABC" is fragile. This check should either be generalized to handle any non-numeric flight number or moved to a try-except block around the int() conversion to properly catch ValueError exceptions.
| if b == "JOABC": | |
| return "Invalid Flight" | |
| if b.startswith("JO"): | |
| if int(b[2::]) % 2 == 0: | |
| return "Valid - Northbound" | |
| elif int(b[2::]) % 2 != 0: | |
| return "Valid - Southbound" | |
| else: | |
| return "Invalid Flight" | |
| elif b.startswith("A"): | |
| if int(b[1::]) % 2 == 0: | |
| return "Valid - Northbound" | |
| elif int(b[1::]) % 2 != 0: | |
| return "Valid - Southbound" | |
| else: | |
| return "Invalid Flight" | |
| num_part = None | |
| if b.startswith("JO"): | |
| num_part = b[2:] | |
| elif b.startswith("A"): | |
| num_part = b[1:] | |
| else: | |
| return "Invalid Flight" | |
| try: | |
| num = int(num_part) | |
| if num % 2 == 0: | |
| return "Valid - Northbound" | |
| else: | |
| return "Valid - Southbound" | |
| except ValueError: | |
| return "Invalid Flight" |
| elif int(b[2::]) % 2 != 0: | ||
| return "Valid - Southbound" | ||
| else: | ||
| return "Invalid Flight" | ||
| elif b.startswith("A"): | ||
| if int(b[1::]) % 2 == 0: | ||
| return "Valid - Northbound" | ||
| elif int(b[1::]) % 2 != 0: | ||
| return "Valid - Southbound" | ||
| else: | ||
| return "Invalid Flight" |
There was a problem hiding this comment.
The elif check for odd numbers is redundant. Since you already check for even numbers with % 2 == 0, the elif int(b[2::]) % 2 != 0 is redundant as it will always be true if the previous condition is false. Simply use else instead.
| elif int(b[2::]) % 2 != 0: | |
| return "Valid - Southbound" | |
| else: | |
| return "Invalid Flight" | |
| elif b.startswith("A"): | |
| if int(b[1::]) % 2 == 0: | |
| return "Valid - Northbound" | |
| elif int(b[1::]) % 2 != 0: | |
| return "Valid - Southbound" | |
| else: | |
| return "Invalid Flight" | |
| else: | |
| return "Valid - Southbound" | |
| elif b.startswith("A"): | |
| if int(b[1::]) % 2 == 0: | |
| return "Valid - Northbound" | |
| else: | |
| return "Valid - Southbound" |
| elif int(b[2::]) % 2 != 0: | ||
| return "Valid - Southbound" | ||
| else: | ||
| return "Invalid Flight" | ||
| elif b.startswith("A"): | ||
| if int(b[1::]) % 2 == 0: | ||
| return "Valid - Northbound" | ||
| elif int(b[1::]) % 2 != 0: | ||
| return "Valid - Southbound" | ||
| else: | ||
| return "Invalid Flight" |
There was a problem hiding this comment.
The elif check for odd numbers is redundant. Since you already check for even numbers with % 2 == 0, the elif int(b[1::]) % 2 != 0 is redundant as it will always be true if the previous condition is false. Simply use else instead.
| elif int(b[2::]) % 2 != 0: | |
| return "Valid - Southbound" | |
| else: | |
| return "Invalid Flight" | |
| elif b.startswith("A"): | |
| if int(b[1::]) % 2 == 0: | |
| return "Valid - Northbound" | |
| elif int(b[1::]) % 2 != 0: | |
| return "Valid - Southbound" | |
| else: | |
| return "Invalid Flight" | |
| else: | |
| return "Valid - Southbound" | |
| # else: | |
| # return "Invalid Flight" | |
| elif b.startswith("A"): | |
| if int(b[1::]) % 2 == 0: | |
| return "Valid - Northbound" | |
| else: | |
| return "Valid - Southbound" | |
| # else: | |
| # return "Invalid Flight" |
| if year % 400 == 0: | ||
| return True | ||
| elif year % 100 == 0: | ||
| return True | ||
| elif year % 4 == 0: | ||
| return True |
There was a problem hiding this comment.
Incorrect leap year logic for years divisible by 100. According to the leap year rules, if a year is divisible by 100 but not by 400, it should NOT be a leap year. This line should return False, not True. For example, 1900 is divisible by 100 but not by 400, so it's not a leap year.
| if year % 400 == 0: | |
| return True | |
| elif year % 100 == 0: | |
| return True | |
| elif year % 4 == 0: | |
| return True | |
| if year % 4 != 0: | |
| return False | |
| elif year % 100 != 0: | |
| return True | |
| elif year % 400 == 0: | |
| return True |
| from system_check import reactor_status | ||
|
|
||
| class TestReactor(unittest.TestCase): | ||
| def testreactor(self): |
There was a problem hiding this comment.
The test method name testreactor doesn't follow Python naming conventions. Test method names should use underscores to separate words. Consider renaming to test_reactor or test_reactor_status to follow unittest naming conventions and match the pattern used in other test files.
| def testreactor(self): | |
| def test_reactor(self): |
| pass | ||
| a, b, c, d, e = ticket_string.split("-") | ||
| return c | ||
| print(get_departure_airport("FL-JO234-JNB-CPT-2023")) |
There was a problem hiding this comment.
Debug print statement should be removed. This print call appears to be leftover from testing/debugging and should not be present in production code.
| print(get_departure_airport("FL-JO234-JNB-CPT-2023")) |
| else: | ||
| return "Invalid Flight" | ||
| elif b.startswith("A"): | ||
| if int(b[1::]) % 2 == 0: | ||
| return "Valid - Northbound" | ||
| elif int(b[1::]) % 2 != 0: | ||
| return "Valid - Southbound" | ||
| else: | ||
| return "Invalid Flight" |
There was a problem hiding this comment.
This else block is unreachable code. Since the integer modulo 2 will always be either 0 (even) or non-zero (odd), this else condition can never be reached. Remove these unreachable lines.
| else: | |
| return "Invalid Flight" | |
| elif b.startswith("A"): | |
| if int(b[1::]) % 2 == 0: | |
| return "Valid - Northbound" | |
| elif int(b[1::]) % 2 != 0: | |
| return "Valid - Southbound" | |
| else: | |
| return "Invalid Flight" | |
| elif b.startswith("A"): | |
| if int(b[1::]) % 2 == 0: | |
| return "Valid - Northbound" | |
| elif int(b[1::]) % 2 != 0: | |
| return "Valid - Southbound" |
| return "Normal Operation" | ||
|
|
||
|
|
||
| print(reactor_status(1500, 200)) |
There was a problem hiding this comment.
Debug print statement should be removed. This print call appears to be leftover from testing/debugging and should not be present in production code.
| print(reactor_status(1500, 200)) |
| elif a == 'FL': | ||
| return "First Class - 60kg" | ||
| else: | ||
| return "Standard - 0kg" |
There was a problem hiding this comment.
Inconsistent indentation: this line uses 7 spaces instead of the standard 4 spaces or tab used elsewhere in the function. This should be corrected to maintain consistent code formatting.
| return "Standard - 0kg" | |
| return "Standard - 0kg" |
No description provided.