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

Complete PFA DSL Library #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Paxanator
Copy link

Address #7

@@ -57,6 +58,19 @@ class CoreLibrarySuite extends FunctionLibrarySuite {
assert(engine.action(engine.jsonInput("4.0")) == 2.0)
}

test("Core divfloor") {
Copy link
Author

Choose a reason for hiding this comment

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

These tests were added by hand before i did the code gen thing

@@ -1,7 +1,7 @@
package com.ibm.aardpfark.pfa.expression

import com.ibm.aardpfark.pfa.document.{PFAExpressionSerializer, ParamSerializer, SchemaSerializer}
import org.json4s.native.JsonMethods.parse
import org.json4s.native.JsonMethods.{parse => parseJSON}
Copy link
Author

Choose a reason for hiding this comment

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

the dsl._ namespace has parse.{int,float..} functions that conflict with JsonMethods

@Paxanator
Copy link
Author

I ran into a bunch of conflicts where either:

  • FunctionRef was the param type, but FunctionDef was supplied
  • Another type was supplied that was not FunctionRef

Overall, I would like to understand whether the DSL should be completely loosely types, or as strongly types as possible, and that's future work we should include

@Paxanator
Copy link
Author

The build is failing because of some maven key thing, any insight into how to fix it?

@Paxanator
Copy link
Author

I ran into a bunch of conflicts where either:

  • FunctionRef was the param type, but FunctionDef was supplied
  • Another type was supplied that was not FunctionRef

Overall, I would like to understand whether the DSL should be completely loosely types, or as strongly types as possible, and that's future work we should include

Additionally there are PFAExpressions that were peppered through that provide implicit casting of strings to StringLiterals, so some tests are failing. Waiting to hear on the above to resolve it

@Paxanator
Copy link
Author

@MLnick any insight into the above questions?

@MLnick
Copy link
Collaborator

MLnick commented Jan 9, 2019 via email

@MLnick
Copy link
Collaborator

MLnick commented Jan 24, 2019

@Paxanator overall the approach looks good and I like the code gen for functions. It would be interesting to also see whether we could code gen the test cases based on the PFA conformance tests JSON (but that may be more involved).

Overall ideally I think the DSL would be as strongly-typed as possible, to try to avoid runtime errors at conversion time (i.e. generating invalid PFA, that can only be caught running integration / conformance tests or trying to execute the PFA). As you mention it's not very close to that goal as yet but would be good to work towards it. This is why many of the functions (in most cases the ones we added later on in development) try to take PFAExpression as args - the implicit casts from common simple types then make it easier to use Scala strings, numbers etc in the DSL in raw form.

Of course, this does introduce more "magic" and there's an argument that being more explicit and requiring users to use StringLiteral, DoubleLiteral, makes it more clear what is going on under the hood.

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