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

feat: support has() macro #33

Merged
merged 1 commit into from
Dec 15, 2024
Merged

Conversation

Jastrzebowski
Copy link
Contributor

@Jastrzebowski Jastrzebowski commented Dec 13, 2024

Describe your changes

I'm adding support for the has() macro. I'm providing it similarly to the existing size() macro. I did copy the GO style of the macro has(object.property) to maximise interoperability.

A big part of supporting has() is done in a few different visitors:

  1. macrosExpression supports has() as a special case and calls for a helper method to handle it. For any other macro, the process continues to work the same.
  2. handleHasMacro — handles the has() case, trying to evaluate the argument provided to the macro. The method will throw if no arguments are passed. The method also sets up an inHas context, which visitors later use to change the default behaviour if called in a has() macro—The context approach mimics GO implementation. I'm not a GO developer, so I did my best to understand how they deal with this. The method will rethrow any exceptions thrown by called visitors; this is how we deal with invalid arguments.
  3. conditionalAnd — I've added a short circuit for cases where the left side operand is false. This allows to use has as a check for existence:
has(object.nonExisting) && object.nonExisting
  1. atomicExpression — Will throw when called in the in context and evaluate an identifierExpression.
  2. identifierExpression — This is the only allowed case for a has() macro. It still will throw if not called for an identifierDotExpression context.

Expected success cases

const context = { object: { property: true } }

evaluate('has(object.property)', context) // true
evaluate('has(object.nonExisting)', context) // false
evaluate('has(object.nonExisting) && object.nonExisting', context) // false

Expected errors

const context = { object: { property: true } }

evaluate('has()', context)
evaluate('has(object)', context)
evaluate('has(1)', context)
evaluate('has("string")', context)
evaluate('has([])', context)
evaluate('has([1,2,3])', context)
evaluate('has(object[0])', context)

Disclaimer

I used the AWS Q AI assistant while working on the implementation and documentation. I reviewed the proposed code.

Issue ticket number/link

#32

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have added tests (if possible)
  • I have updated the readme (if necessary)
  • I have updated the demo (if necessary)

@Jastrzebowski
Copy link
Contributor Author

I will update the README and demo once the proposed changes are accepted.

@ChromeGG
Copy link
Owner

I've reviewed the code. GJ!
There is one issue, context should be dedicated for user input only. We should not mix evaluation state there, there could be conflict with user's variables. Let's try to fix this and some linting errors. Then I'm happy to merge it :)

@Jastrzebowski
Copy link
Contributor Author

I saw that the context is used for other purposes, and I agree it would be better not to mix them. We should specify the type better if we intend to use named properties, so a separate property would help us with that too. I will introduce a mode property to store the mode in which a node is visited.

@Jastrzebowski
Copy link
Contributor Author

I pushed my changes. Please review the Readme, I used the macros notation defined in the language definition. As for the lint error it had to do with my initial commit message. I missed that you are following the conventional commits format. The local yarn lint is passing without errors.

src/visitor.ts Show resolved Hide resolved
@ChromeGG ChromeGG changed the title Adding Has() macro feat: support has() macro Dec 15, 2024
@ChromeGG ChromeGG enabled auto-merge (squash) December 15, 2024 11:36
@ChromeGG ChromeGG disabled auto-merge December 15, 2024 12:00
@ChromeGG ChromeGG merged commit a9d43b3 into ChromeGG:main Dec 15, 2024
2 of 3 checks passed
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.

2 participants