Skip to content

Conversation

@ehuelsmann
Copy link
Contributor

@ehuelsmann ehuelsmann commented Oct 18, 2021

Description

Refactor the code to comply with the shared tests for expressions and errors.

Motivation & context

Consistency across implementations.

Type of change

  • Refactoring/debt (improvement to code design or tooling without changing behaviour)

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • I have added tests to cover my changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

In order to match the expected error texts, change existing failure tests
and remove tests for "empty tags" after discussion with @aslakhellesoy as
to whether it's desirable that the tag-expressions library has knowledge
of tokens being tags (it's not).
@ehuelsmann
Copy link
Contributor Author

@aslakhellesoy The Perl implementation has a different internal representation, which causes differences in the "parsing.yml" output: the "and" and "or" operators are "lispy" in the sense that they accept as many operands as you provide, resulting in:

# Failed test 'Stringified parser for \\x or y\\ or z\\'
# at t/03-shared-tests.t line 63.
# +-----------------------+----+---------------------------+
# | GOT                   | OP | CHECK                     |
# +-----------------------+----+---------------------------+
# | ( \\x or y\\ or z\\ ) | eq | ( ( \\x or y\\ ) or z\\ ) |
# +-----------------------+----+---------------------------+

Although I do see the point in the parsing.yml tests (and I found it helpful in discovering that '' is defined to return true always), I'm wondering if running these should be more than a developer's tool to inspect that the reported differences have no semantic impact.

Fix the "empty string" equals 'constant true' issue found by studying
parsing.yml. Also use infix stringification to closer resemble the
expected test output (but not fully: our 'or' accepts more than 2 operands).
@ehuelsmann
Copy link
Contributor Author

Actually, I just figured out how to compensate the difference without changing the internal representation.

Copy link
Contributor

@aslakhellesoy aslakhellesoy left a comment

Choose a reason for hiding this comment

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

Very nice @ehuelsmann! These shared acceptance tests seem to come in handy :-)

@ehuelsmann
Copy link
Contributor Author

Very nice @ehuelsmann! These shared acceptance tests seem to come in handy :-)

Absolutely. They give me the confidence that we present the same API across implementations.

@ehuelsmann ehuelsmann linked an issue Oct 18, 2021 that may be closed by this pull request
2 tasks
@ehuelsmann ehuelsmann merged commit 289b587 into main Oct 18, 2021
@ehuelsmann ehuelsmann deleted the perl-shared-tests branch October 18, 2021 21:47
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.

Perl: Make tests pass against shared test data

3 participants