Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not.to_string() conversion has unneeded double-parenthesis if the inner expression is a binary operator #94

Closed
jenisys opened this issue May 28, 2023 · 0 comments · Fixed by #95

Comments

@jenisys
Copy link
Contributor

jenisys commented May 28, 2023

👓 What did you see?

If a tag-expression contains a not operator that encapsulate a binary operator (and, or),
the string conversion currently leads to double-parenthesis (unnecessary tautology).
This makes the textual string less readable and understandable.

EXAMPLE:

# SOURCE: testdata/parsing.yml
# HINT: This data or similar examples are currently missing

- expression: 'not (a and b)'
  formatted.currently: 'not ( ( a and b ) )'
  formatted.should_be: 'not ( a and b )'

REASONS:

  • The binary operators put already parenthesis around their boolean expression during string conversion
  • If the not operator does the same (as it currently is implemented), we get two groups of parenthensis

AFFECTED:

  • ALL tag-expression implementations

✅ What did you expect to see?

Only one parenthesis grouping, like the formatted.should_be case shown in the example above.

📦 Which tool/library version are you using?

I added additional case to the testdata/parsing.yml in my local workspace and run Java, Javascript and Go implementations.
Each failed the new additional tests.
The non-tested implementations (Ruby and Perl) are also be affected (as expected), by inspection of their implementation.

🔬 How could we reproduce it?

Add the following test-data items to testdata/parsing.yml:

# SOURCE: testdata/parsing.yml

# BAD_EXAMPLES (how the string output currently is):

- expression: 'not (a and b)'
  formatted: 'not ( ( a and b ) )'
- expression: 'not (a or b)'
  formatted: 'not ( ( a or b ) )'
- expression: 'not (a and b) and c or not (d or f)'
  formatted: '( ( not ( ( a and b ) ) and c ) or not ( ( d or f ) ) )'

# GOOD_EXAMPLES (how the output should be):

- expression: 'not (a and b)'
  formatted: 'not ( a and b )'
- expression: 'not (a or b)'
  formatted: 'not ( a or b )'
- expression: 'not (a and b) and c or not (d or f)'
  formatted: '( ( not ( a and b ) and c ) or not ( d or f ) )'

Steps to reproduce the behavior:

  1. Update the local workspace to the current HEAD of the GitHub:/cucumber/tag-expression repository.
  2. Patch the testdata file
  3. Run the tests for each implementation

📚 Any additional context?

Nope.

jenisys added a commit to jenisys/cucumber.tag-expressions that referenced this issue May 28, 2023
IMPLEMENTED FIX FOR:

* [x] java
* [x] javascript
* [x] python
jenisys added a commit to jenisys/cucumber.tag-expressions that referenced this issue May 29, 2023
* notExpr.ToString()
* ADDED: Evaluatable.isBinaryOperator() (used by: notExpr)
  REASON: Was faster than providing an instance-of implementation
jenisys added a commit to jenisys/cucumber.tag-expressions that referenced this issue May 29, 2023
    * Not.to_s()  -- to_string conversion
jenisys added a commit to jenisys/cucumber.tag-expressions that referenced this issue May 29, 2023
    * Not.to_s()  -- to_string conversion
jenisys added a commit to jenisys/cucumber.tag-expressions that referenced this issue May 29, 2023
    * Not.to_s()  -- to_string conversion
jenisys added a commit to jenisys/cucumber.tag-expressions that referenced this issue May 29, 2023
jenisys added a commit that referenced this issue Jun 2, 2023
* Fixes: #94
* Implements: #18
jenisys added a commit that referenced this issue Jul 2, 2023
* FIXED #94 Not.to_string() conversion leads to double-parenthesis

IMPLEMENTED FIX FOR:

* [x] java
* [x] javascript
* [x] python

* FIX CI-BUILD: Missing dependency PyYAML

* Python 2.7 related issues (pytest, pathlib related)
* FIX: tox to test Python 2.7 usecase locally

UPDATE: Dependencies

* setup.py
* py.requirements/*.txt -- testing and development related

* FIX CI BUILD: prettier formatting issue

* FIXES #94 for Go programming language

* notExpr.ToString()
* ADDED: Evaluatable.isBinaryOperator() (used by: notExpr)
  REASON: Was faster than providing an instance-of implementation

* FIXES #94 for Ruby programming language

    * Not.to_s()  -- to_string conversion

* FIXES #94 for Perl programming language

* go: Provide IsBinaryOperator function

* Remove IsBinaryOperator() method in Evaluatable interface
* Provide IsBinaryOperator() function instead
  that encapsulates the is-instance-of And/Or expression.

* perl: Use isa operator instead of isBinaryOperator method

* Remove Node::isBinaryOperator() method and implementations

* go: Use "isBinaryOperator" (naming style change)

* Use "isBinaryOperator()" (was: "IsBinaryOperator")
  to comply w/ Go naming conventions and avoid to expose
  the this helper function to the public API of this package.

* UPDATED: CHANGELOG for pull #95

* Fixes: #94
* Implements: #18

* CI workflow: test-python -- Remove Python 2.7

* REASON: Github Actions support for Python 2.7 was removed (in 2023-06)
* Try to use pypy-2.7 instead (until Python 2.7 support is dropped)

* CI workflow: test-python -- Remove Python 2.7

* REASON: Github Actions support for Python 2.7 was removed (in 2023-06)
* Try to use pypy-2.7 instead (until Python 2.7 support is dropped)
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 a pull request may close this issue.

1 participant