smithy-lang / smithy-kotlin

Smithy code generator for Kotlin (in development)
Apache License 2.0
77 stars 26 forks source link

feat: add functions (sum, avg, join, starts_with, ends_with) to JMESPath visitor #940

Closed 0marperez closed 1 year ago

0marperez commented 1 year ago

Issue \

Description of changes

Added: -Sum -Avg -Join -Starts with -Ends with

Refactored: -Separated operations from structures in smithy file -Wrote function to avoid code duplication in JMESPath visitor functions code

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

0marperez commented 1 year ago

About the avg function test for an empty list (EmptyIntegerListAvgNotEquals). JMESPath doesn't have a way to check if something is null so I just compared it to a number and set the success criteria to false. You can verify this behavior by playing around with JMESPath on their website and I also tried it in a waiter btw.

ianbotsf commented 1 year ago

JMESPath doesn't have a way to check if something is null...

I don't understand. The spec's section on equality seems to indicate that null is only equal to itself. You weren't able to get an equality path to pass booleanEquals?

matcher: {
    output: {
        path: "avg(lists.integers) == null",
        expected: "true",
        comparator: "booleanEquals"
    }
}
0marperez commented 1 year ago

You weren't able to get an equality path to pass booleanEquals?

No, when trying it in the waiter I get an error message:

6 | @waitable(
  | ^
··|
94| operation GetFunctionAvgEquals {

Waiter `EmptyIntegerListAvgEquals`, acceptor 0: Problem found in JMESPath
expression (avg(lists.integers) == null): Object field 'null' does not exist in
object with properties [primitives, lists, twoDimensionalLists, maps] (1:24)

And in the JMESPath website you can see null is there:

Screenshot 2023-08-30 at 10 25 46 AM

But when comparing it to another null it doesn't work:

Screenshot 2023-08-30 at 10 26 02 AM Screenshot 2023-08-30 at 10 26 17 AM
sonarcloud[bot] commented 1 year ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

ianbotsf commented 1 year ago

No, when trying it in the waiter I get an error message:

6 | @waitable(
  | ^
··|
94| operation GetFunctionAvgEquals {

Waiter `EmptyIntegerListAvgEquals`, acceptor 0: Problem found in JMESPath
expression (avg(lists.integers) == null): Object field 'null' does not exist in
object with properties [primitives, lists, twoDimensionalLists, maps] (1:24)

Interesting. It looks like Smithy's JMESPath type checker is getting to visitField when I'd expect it to be hitting visitLiteral. I suspect this might be a bug in Smithy and I recommend you file an issue to them.

Looks like what you've got is the best we can do for now!