Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive tests for LALR grammar parsing and improves type hints in the parsing module. The PR creates two new test files that demonstrate the differences between SLR, LALR1, and LR1 parsers, adds support for the new has_conflicts property to detect parser conflicts, and fixes naming issues in existing tests.
Changes:
- Added two new test files demonstrating grammars that are LR1 but not LALR, and LALR1 but not SLR
- Enhanced type hints in
pyjapt/parsing.pywithLiteraltypes for parser names and return types for helper functions - Fixed spelling in
get_arithmetic_expressions_grammarfunction name and added test coverage for all three parser types - Added pytest markers for lr1 and lalr1 tests, and updated configuration accordingly
- Refactored version extraction in
build.pyto use direct import instead of regex parsing
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_lr1_but_not_lalr_grammar.py | New test file demonstrating a grammar that LR1 can parse but LALR1 cannot |
| tests/test_lalr1_but_not_slr.py | New test file demonstrating a grammar that LALR1 can parse but SLR cannot |
| tests/test_arithmetic_grammar.py | Fixed function name spelling/clarity, added Literal type hints, and added lr1/lalr1 test functions |
| pyproject.toml | Added pytest markers for lr1 and lalr1 parser tests |
| pyjapt/parsing.py | Added Literal type hints for parser selection, return type annotations for helper functions, and has_conflicts property |
| build.py | Simplified version extraction by importing pyjapt directly instead of regex parsing |
| .vscode/settings.json | Removed IDE-specific settings file |
| .gitignore | Added .vscode/settings.json to ignore IDE-specific files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return g | ||
|
|
||
|
|
||
| def test_slr(): |
There was a problem hiding this comment.
Missing pytest marker for SLR test. Similar to the pattern in test_arithmetic_grammar.py, this test function should have the @pytest.mark.slr decorator to allow selective test execution. You'll also need to import pytest at the top of the file.
| assert parser.has_conflicts | ||
|
|
||
|
|
||
| def test_lalr(): |
There was a problem hiding this comment.
Missing pytest marker for LALR1 test. Similar to the pattern in test_arithmetic_grammar.py (lines 71-74), this test function should have the @pytest.mark.lalr1 decorator to allow selective test execution. You'll also need to import pytest at the top of the file.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| return g | ||
|
|
||
|
|
||
| def test_lalr(): |
There was a problem hiding this comment.
Missing pytest marker for LALR1 test. Similar to the pattern in test_arithmetic_grammar.py (lines 71-74), this test function should have the @pytest.mark.lalr1 decorator to allow selective test execution. You'll also need to import pytest at the top of the file.
| assert parser.has_conflicts | ||
|
|
||
|
|
||
| def test_lr1(): |
There was a problem hiding this comment.
Missing pytest marker for LR1 test. Similar to the pattern in test_arithmetic_grammar.py (lines 65-68), this test function should have the @pytest.mark.lr1 decorator to allow selective test execution. You'll also need to import pytest at the top of the file.
build.py
Outdated
| with open("pyjapt/__init__.py", "r") as f: | ||
| version = ( | ||
| re.search(r"__version__ = \"\d\.\d\.\d\"", f.read()) | ||
| .group() | ||
| .replace('__version__ = "', "")[:-1] | ||
| ) | ||
| version = pyjapt.__version__ |
There was a problem hiding this comment.
Unnecessary file read operation. The file is opened but never read, since the version is now obtained directly from pyjapt.version. This entire with block (lines 6-7) can be replaced with just version = pyjapt.__version__ since the file reading is redundant.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@alejandroklever I've opened a new pull request, #13, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@alejandroklever I've opened a new pull request, #14, to work on those changes. Once the pull request is ready, I'll request review from you. |
…not_slr.py Co-authored-by: alejandroklever <45394625+alejandroklever@users.noreply.github.com>
…ctly Co-authored-by: alejandroklever <45394625+alejandroklever@users.noreply.github.com>
Remove redundant file open in build.py
Add @pytest.mark.lalr1 marker to test_lalr in test_lalr1_but_not_slr.py
No description provided.