bug(#551): remove QQ from phino
#553
Conversation
WalkthroughThis PR removes the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (36)
💤 Files with no reviewable changes (9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (36)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/XMIR.hs (1)
101-128: XMIR serialization for DataNumber and DataString lacks corresponding parsing support—this breaks round-trip consistency.The serialization side correctly produces
"Φ.number"and"Φ.string"identifiers, but the parsing logic (xmirToExpression) treats these as generic dispatch expressions instead of reconstructing the originalDataNumberandDataStringtypes. ThexmirToExpression'function splits on"."and buildsExDispatchchains without special handling for"number"or"string"suffixes, causing loss of type information during parsing.To maintain round-trip consistency, the parsing logic needs to detect and reconstruct
DataNumberandDataStringpatterns from the simplifiedΦ.numberandΦ.stringbase paths, similar to how the serialization currently distinguishes them during expression writing.src/Dataize.hs (1)
212-243: Update README.md with new atom names.The atom name changes in src/Dataize.hs are correctly applied, but README.md still references the old names
L_org_eolang_number_plus(lines 177, 199). These must be updated toL_number_plusto maintain consistency with the PR objective of movingQ.org.eolangtoQ.
- README.md:177 — change
L_org_eolang_number_plustoL_number_plus- README.md:199 — change
L_org_eolang_number_plustoL_number_plus
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
README.md(2 hunks)phino.cabal(0 hunks)src/CST.hs(0 hunks)src/Dataize.hs(3 hunks)src/Encoding.hs(0 hunks)src/Misc.hs(1 hunks)src/Parser.hs(0 hunks)src/Render.hs(0 hunks)src/Sugar.hs(0 hunks)src/XMIR.hs(2 hunks)test-resources/cli/desugar.phi(1 hunks)test-resources/cst/printing-packs/jeo.yaml(2 hunks)test-resources/cst/printing-packs/primitives-without-rhos.yaml(1 hunks)test-resources/cst/to-ascii-packs/complex.yaml(2 hunks)test-resources/cst/to-salty-packs/application-with-alphas.yaml(1 hunks)test-resources/cst/to-salty-packs/application.yaml(1 hunks)test-resources/cst/to-salty-packs/default-package.yaml(0 hunks)test-resources/cst/to-salty-packs/primitives-with-rhos.yaml(2 hunks)test-resources/cst/to-salty-packs/with-num-and-void-rho.yaml(1 hunks)test-resources/parser-packs/all-the-basics.phi(1 hunks)test-resources/parser-packs/ascii-with-braces.phi(1 hunks)test-resources/parser-packs/ascii-with-global.phi(1 hunks)test-resources/parser-packs/salty-fibo.phi(1 hunks)test-resources/parser-packs/simple.phi(1 hunks)test-resources/rewriter-packs/custom/desugar-fibo.yaml(2 hunks)test-resources/rewriter-packs/custom/desugar-strings.yaml(1 hunks)test-resources/rewriter-packs/custom/desugares-without-match.yaml(1 hunks)test-resources/rewriter-packs/custom/desugares.yaml(1 hunks)test-resources/rewriter-packs/custom/java-boxed-method.yaml(2 hunks)test-resources/xmir-printing-packs/application.yaml(1 hunks)test-resources/xmir-printing-packs/simple.yaml(1 hunks)test/CLISpec.hs(4 hunks)test/DataizeSpec.hs(4 hunks)test/LiningSpec.hs(0 hunks)test/ParserSpec.hs(1 hunks)test/SugarSpec.hs(0 hunks)
💤 Files with no reviewable changes (9)
- src/Encoding.hs
- test-resources/cst/to-salty-packs/default-package.yaml
- src/CST.hs
- test/SugarSpec.hs
- src/Sugar.hs
- src/Render.hs
- src/Parser.hs
- test/LiningSpec.hs
- phino.cabal
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: cabal (windows-2022)
- GitHub Check: cabal (macos-15)
- GitHub Check: stack (windows-2022)
- GitHub Check: stack (macos-15)
🔇 Additional comments (36)
test-resources/rewriter-packs/custom/java-boxed-method.yaml (2)
52-52: LGTM! Pattern updated to match new input syntax.The pattern's
𝜏-flagbinding correctly reflects the updated syntaxΦ.true, ensuring it matches the corresponding input values at lines 15 and 22.
15-15: Namespace simplification applied consistently across test inputs and rules.The change from
Φ̇.truetoΦ.trueis applied correctly at lines 15 and 22 in the input sequences (i95 and i96) and at line 52 in the rules pattern. No remaining references to the old syntax in YAML test files.test-resources/cst/to-salty-packs/application-with-alphas.yaml (1)
10-26: LGTM! Namespace simplification is consistent.The updates from
Φ.org.eolang.numbertoΦ.numberandΦ.org.eolang.bytestoΦ.bytesalign with the PR's objective to simplify public API paths. The test expectations correctly reflect the new base paths.test-resources/xmir-printing-packs/simple.yaml (1)
20-20: LGTM! XPath expectation updated correctly.The XPath condition update from
Φ.org.eolang.numbertoΦ.numbercorrectly aligns with the namespace simplification across the codebase.test-resources/parser-packs/ascii-with-global.phi (1)
12-12: LGTM! QQ removal is consistent.The update from
QQ.txt.sprintftoQ.txt.sprintfcorrectly implements the PR's objective to remove thetest-resources/rewriter-packs/custom/desugares.yaml (1)
13-17: LGTM! Desugaring expectations updated correctly.The desugaring output correctly reflects the simplified namespace paths (
Φ.numberandΦ.bytes), maintaining the correct structure while adopting the new base identifiers.test-resources/parser-packs/all-the-basics.phi (1)
37-37: LGTM! Dotted variant removed correctly.The change from
Φ̇.number(42)toΦ.number(42)correctly standardizes on the non-dotted notation, aligning with the PR's namespace simplification objectives.test-resources/cst/printing-packs/jeo.yaml (1)
22-22: LGTM! Dotted variant consistently removed.Both occurrences of
Φ̇.bytescorrectly updated toΦ.bytes, standardizing the notation throughout the test expectations.Also applies to: 38-38
test-resources/cst/to-salty-packs/with-num-and-void-rho.yaml (1)
9-16: LGTM! Namespace paths simplified correctly.The test expectations correctly reflect the simplified namespace paths from
Φ.org.eolang.numberandΦ.org.eolang.bytestoΦ.numberandΦ.bytes, maintaining correct structure and argument mappings.test-resources/parser-packs/simple.phi (1)
1-1: Verify alignment with PR objectives: should this beQinstead ofQ.org.eolang?The PR objectives state "Move Q.org.eolang to Q" and "remove QQ syntax entirely." However, this change replaces
Q.org.eolangrather than simplyQ. Please confirm whether this intermediate form is intentional or if the mapping should be simplified to justQto fully align with the stated objectives.#!/bin/bash # Verify if Q.org.eolang is used elsewhere or if Q is the target rg -n "Q\.org\.eolang" --type yaml rg -n "foo ↦ Q[^.]" --type yamltest-resources/rewriter-packs/custom/desugar-strings.yaml (1)
14-38: LGTM! Namespace simplification is consistent.The changes correctly update all string and bytes constructor references from the qualified
Φ.org.eolang.*paths to the simplifiedΦ.*paths. The internal structure and byte values remain unchanged, which is correct for this refactoring.test-resources/rewriter-packs/custom/desugares-without-match.yaml (1)
13-17: LGTM! Number constructor path correctly simplified.The namespace update from
Φ.org.eolang.numbertoΦ.numberandΦ.org.eolang.bytestoΦ.bytesis consistent with the PR objective. The test data and custom rules remain properly structured.test-resources/cst/to-salty-packs/application.yaml (1)
10-26: LGTM! Application test case correctly updated.All constructor references have been properly simplified from
Φ.org.eolang.*toΦ.*. The test structure, including alpha attribute mappings and byte values, remains intact as expected.test-resources/cst/to-ascii-packs/complex.yaml (2)
17-17: LGTM! ASCII representation correctly updated.The changes properly reflect:
- Removal of the dotted Φ variant (Φ̇ → Φ)
- Simplification of QQ to Q in ASCII format
Both transformations align with the PR objectives.
Also applies to: 30-30
9-9:orgin line 9 is a user-defined binding, not a namespace reference.The pattern
Φ.org.ρ.φcorrectly represents a binding namedorgwith attributesρandφaccessed on it. This is confirmed by the expected output on line 25 (b -> Q.org.^.@), which maintainsorgas a binding while using different syntax to access its attributes. This is intentional.README.md (2)
142-149: LGTM! Documentation example correctly updated.The rewrite example now demonstrates the simplified namespace paths (
Φ.io.stdout,Φ.string,Φ.bytes), which accurately reflects the implementation changes. This will help users understand the new API structure.
312-312: LGTM! Function documentation updated.The
concatfunction description correctly reflects the new simplified path format for the resulting expression.test-resources/rewriter-packs/custom/desugar-fibo.yaml (1)
28-55: LGTM! Fibonacci test case correctly refactored.All three number literal desugaring instances have been properly updated from
Φ.org.eolang.number/Φ.org.eolang.bytesto the simplifiedΦ.number/Φ.bytespaths. The complex nested structure of the Fibonacci example is preserved, with only the namespace paths updated as intended.src/Misc.hs (1)
78-86: Verify complete removal of nested org.eolang dispatch paths.The simplification of
matchBaseObjectfrom a nested three-level dispatch patternExDispatch (ExDispatch (ExDispatch ExGlobal "org") "eolang") labelto a direct single-level patternExDispatch ExGlobal labelis complete. No code references the old nested dispatch structure for this function. Nested dispatch patterns in test files are legitimately used for testing other functions likefqnToAttrs,ParserSpec, andMatcherSpec, which handle arbitrary dispatch nesting depths independent ofmatchBaseObject.test-resources/cst/to-salty-packs/primitives-with-rhos.yaml (2)
7-11: LGTM! Consistent namespace simplification.The input program correctly updates all references from
QQ.*toQ.*, consistently applying the namespace simplification acrossnumber,bytes, andstringconstructors.
16-29: LGTM! Result expectations properly updated.The expected transformation output correctly reflects the new simplified base paths (
Φ.number,Φ.bytes,Φ.stringinstead ofΦ.org.eolang.*), ensuring test expectations align with the refactored public API.test-resources/parser-packs/ascii-with-braces.phi (1)
13-13: LGTM! Namespace prefix updated correctly.Both references correctly migrated from
QQ.*toQ.*syntax (txt.sprintfandio.text), aligning with the removal ofAlso applies to: 17-17
test-resources/cli/desugar.phi (1)
3-3: LGTM! Test input updated to remove QQ reference.The mapping changed from
x(regular identifier), which correctly removes dependency on the deprecatedtest-resources/parser-packs/salty-fibo.phi (1)
8-11: LGTM! Namespace paths simplified consistently.All numeric literal constructions in the fibonacci implementation correctly updated from
Φ.org.eolang.numberandΦ.org.eolang.bytesto the simplifiedΦ.numberandΦ.bytesforms. The changes maintain the same structure and semantics while using the new public API paths.Also applies to: 17-21, 26-31
test-resources/xmir-printing-packs/application.yaml (1)
14-15: LGTM! XPath assertions updated for new base identifiers.The XPath selectors correctly updated to match the simplified base attributes (
Φ.numberandΦ.stringinstead ofΦ.org.eolang.*), ensuring XMIR output validation aligns with the refactored namespace structure.test/ParserSpec.hs (1)
218-218: LGTM! Proper test coverage for QQ prohibition.The test correctly adds
test-resources/cst/printing-packs/primitives-without-rhos.yaml (1)
7-8: LGTM! Input program updated to use Q namespace.The test input correctly migrates from
QQ.*toQ.*syntax fornumber,bytes, andstringconstructors, consistent with the namespace simplification across the repository.test/DataizeSpec.hs (4)
128-143: LGTM! Fahrenheit test updated with simplified atoms.Test expectations correctly updated with simplified
bytesandnumberconstructors and atom labels (L_number_plus,L_number_times), aligning with the namespace simplification throughout the codebase.
149-171: LGTM! Factorial test simplified consistently.All atom labels properly updated to use the simplified naming scheme (
L_number_times,L_number_plus,L_number_eq), matching the pattern established in other test cases.
192-198: LGTM! Five test uses simplified constructors.The test correctly uses the simplified
number(as-bytes)andbytes(data)constructor forms, maintaining consistency with the broader namespace refactoring.
111-121: LGTM! Test structure simplified for clarity.The test program structure correctly updated to use simplified constructors (
bytes(data)andnumber(as-bytes)) instead of nestedorg.eolangpaths, with atom labels updated toL_number_plus,L_number_times, andL_number_eq. These labels are properly mapped in src/Dataize.hs with correct dataization implementations, maintaining equivalent semantics while improving readability.test/CLISpec.hs (4)
264-264: Test expectations correctly updated for QQ removal.The updated expectations now reflect the simplified desugaring output (
ξ.xinstead ofΦ.org.eolang.*), consistent with the PR objective of removingAlso applies to: 267-268
318-323: LaTeX output expectations correctly updated.The change from
QQ.ztoQ.zin the expected LaTeX output aligns with the removal of the
746-746: Merge test expectation updated.The expected output
{⟦ foo ↦ x ⟧}reflects the simplified representation after QQ removal.
611-611: Node count change in debug output is accurate for COPY transformation.The reduction from 20 → 17 nodes (delta of 3 nodes) in the debug output is correct. This is the only node count change present in the test expectation.
Likely an incorrect or invalid review comment.
src/Dataize.hs (1)
113-113: Simplified global dispatch handling.The new pattern directly handles
ExDispatch ExGlobal (AtLabel label)by invokingphiDispatch, which aligns with the simplified namespace structure after QQ removal. This is cleaner than the previous nested dispatch pathway.
|
@rultor merge |
@maxonfjvipon OK, I'll try to merge now. You can check the progress of the merge here. |
@maxonfjvipon Done! FYI, the full log is here (took me 11min). |
2 participants
Closes: #551
Summary by CodeRabbit
Release Notes
Documentation
Φ.org.eolang.string()→Φ.string(),Φ.org.eolang.bytes()→Φ.bytes(),Φ.org.eolang.number()→Φ.number().Tests
✏️ Tip: You can customize this high-level summary in your review settings.