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

Add validation for If node #1177

Merged
merged 2 commits into from
Jul 30, 2024
Merged

Add validation for If node #1177

merged 2 commits into from
Jul 30, 2024

Conversation

kiri11
Copy link
Contributor

@kiri11 kiri11 commented Jul 27, 2024

Don't allow no space no parentheses after if.

Summary

There should be a validation for If node similar to others such as While or Assert to not allow generating incorrect code.
There is even a TODO left for that reason.

Let me know if there should be other validations for If.

Test Plan

Before:

With the current version of LibCST it is possible to generate the code like this with an If node followed by Name('x') without space:

ifx:
    pass

which isn't correct Syntax.

The new test fails:

======================================================================
FAIL: test_invalid_0 (libcst._nodes.tests.test_if.IfTest.test_invalid_0)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "libcst\testing\utils.py", line 88, in new_test
    return member(self, *data)
           ^^^^^^^^^^^^^^^^^^^
  File "libcst\_nodes\tests\test_if.py", line 149, in test_invalid
    self.assert_invalid(get_node, expected_re)
  File "libcst\_nodes\tests\base.py", line 95, in assert_invalid
    with self.assertRaisesRegex(cst.CSTValidationError, expected_re):
AssertionError: CSTValidationError not raised

After:

Attempting to generate incorrect code raises an exception preventing it:

libcst._nodes.base.CSTValidationError: Must have at least one space after 'if' keyword.

The new test passes:

----------------------------------------------------------------------
Ran 2076 tests in 62.315s

OK (skipped=18, expected failures=2)

Don't allow no space no parentheses.
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 27, 2024
Copy link

codecov bot commented Jul 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.26%. Comparing base (e20e757) to head (3dc3480).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1177   +/-   ##
=======================================
  Coverage   91.26%   91.26%           
=======================================
  Files         261      261           
  Lines       26877    26883    +6     
=======================================
+ Hits        24529    24535    +6     
  Misses       2348     2348           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kiri11 kiri11 marked this pull request as ready for review July 30, 2024 00:22
@zsol
Copy link
Member

zsol commented Jul 30, 2024

✨ Thanks! ✨

@zsol zsol merged commit b0d145d into Instagram:main Jul 30, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants