Skip to content

Comments

swev-id: astropy__astropy-14365 — ascii.qdp: accept case-insensitive QDP commands; add tests#27

Closed
rowan-stein wants to merge 4 commits intoastropy__astropy-14365from
fix/qdp-case-insensitive-14365
Closed

swev-id: astropy__astropy-14365 — ascii.qdp: accept case-insensitive QDP commands; add tests#27
rowan-stein wants to merge 4 commits intoastropy__astropy-14365from
fix/qdp-case-insensitive-14365

Conversation

@rowan-stein
Copy link
Collaborator

Updated the ascii.qdp reader to recognize QDP error command directives (READ SERR/READ TERR) case-insensitively using an inline regex flag scoped only to the commands. Added tests covering uppercase, lowercase, and mixed-case inputs. No changes to data parsing or table splitting tokens. Fixes #26.

Rowan Ellis added 2 commits October 23, 2025 15:39
Scope case-insensitivity to READ [TS]ERR commands via inline regex flag. Add minimal note comment.
Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

Thanks for addressing Issue #26. The approach to scope case-insensitivity via an inline regex flag is sound and should limit impact to command detection. However, the PR currently has blocking issues:

  • _command_re string in qdp.py is syntactically incomplete; the inline flag group appears unclosed.
  • The new test file starts with a leading space and ends with non-Python content (PYTEST and shell commands), which will cause syntax errors.

Please fix these and consider the minor regex improvements suggested. Once addressed, I’m happy to re-review and approve.

Choose a reason for hiding this comment

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

[major] The test file includes non-Python content at the end (lines like PYTEST and shell commands: git add -A, gh pr create, etc.). These will cause syntax errors and must be removed. The file should only contain test code.

Choose a reason for hiding this comment

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

[minor] Nice coverage of uppercase/lowercase/mixed cases and with/without explicit names. To strengthen robustness, consider adding tests for:

  • Tabs or multiple spaces between tokens (e.g., READ\tSERR\t3 or READ TERR 1)
  • Trailing whitespace on directive lines (READ SERR 3 )
  • A comment line following directives to ensure comment detection remains unaffected

These help confirm delimiter handling and that case-insensitive matching doesn’t interfere with other line-type detection.

Choose a reason for hiding this comment

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

[major] The _command_re assignment appears syntactically incomplete. The inline flag group (?i:...) is missing its closing parenthesis, and the string literal lacks a closing quote. This will cause a syntax error and prevent parsing. Please fix to something like:

  • Minimal fix: r"(?i:READ [TS]ERR(\s+[0-9]+)+)"
  • More robust (anchor and allow trailing spaces): r"(?i)^READ [TS]ERR(?:\s+[0-9]+)+\s*$"

The inline flag is appropriately scoped to the command portion, so this change should only affect command matching and leave decimal/data/comment/new-table detection unchanged.

@@ -0,0 +1,95 @@
import numpy as np

Choose a reason for hiding this comment

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

[major] Unexpected indent before top-level imports (leading space at column 1). This will raise IndentationError. Please remove the leading space so imports start at column 0.

Choose a reason for hiding this comment

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

[minor] Consider anchoring the command regex and permitting optional leading/trailing whitespace to avoid false positives/negatives, e.g. r"(?i)^\s*READ [TS]ERR(?:\s+[0-9]+)+\s*$". This prevents a data line that happens to contain the substring from matching, and tolerates trailing spaces which are common in QDP files.

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

Thanks for the update. Verification on the latest commit (82c020b):

  • qdp.py: _command_re now correctly uses an inline, scoped case-insensitive group: (?i:READ\s+[TS]ERR(?:\s+[0-9]+)+). No conflict markers present. Other patterns and line-type logic appear unchanged.
  • Tests: Added coverage for uppercase/lowercase/mixed-case, tabs/multiple spaces, trailing whitespace, and a comment line after directives — assertions look correct. However, the test file currently has top-level indentation (one leading space) before def _make_qdp_text, the @pytest.mark.parametrize decorator, and def test_qdp_command_case_insensitive, which will raise an IndentationError. There are also trailing spaces on blank lines after imports.

Requested fixes:

  1. Remove the leading space at the start of all top-level decorators and def statements so they begin at column 0.
  2. Strip trailing whitespace on blank lines after the import block.

Once these are addressed, I’m ready to approve.

Choose a reason for hiding this comment

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

[nit] The note below the _command_re repeats the case-insensitivity statement already conveyed in the preceding comment. Consider condensing/removing the redundant line to keep comments crisp.

Choose a reason for hiding this comment

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

[nit] Minor style: the two blank lines after the imports contain trailing spaces. Please strip trailing whitespace to satisfy linters.

Choose a reason for hiding this comment

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

[major] Several top-level statements are indented by one space (e.g., def _make_qdp_text(...), the @pytest.mark.parametrize(...) decorator, and the subsequent def test_qdp_command_case_insensitive(...)). Top-level defs/decorators must start at column 0; this will raise an IndentationError and break the test file. Please remove the leading spaces so these lines are flush-left.

@rowan-stein
Copy link
Collaborator Author

Closing to enforce single open PR per task (ID: 10). Please refer to PR #121 for the active work.

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