-
Notifications
You must be signed in to change notification settings - Fork 7
Improve tooling and sbt-plugin error handling #144
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
Conversation
|
||
result match { | ||
case Right(binding) => | ||
binding.writeToFile(outputFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion is to eventually remove error emitting code from these tests so we can add assert(binding.errors.isEmpty)
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we still want to check what bindings are generated if a header file cannot be perfectly converted to Scala code.
For example we check that literal define is skipped if its value does not fit into Scala type.
Even if we check in BindgenReportingSpec
that warning is printed for this case it does not guarantee that the code is not outputted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming these are edge cases it could still make sense to have them in the reporting spec.
@@ -52,7 +53,7 @@ sealed trait Bindgen { | |||
/** | |||
* Run binding generator | |||
*/ | |||
def generate(): Bindings | |||
def generate(): Either[Seq[String], Bindings] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Also maybe update the comment:
/**
* Run binding generator
* @return errors if exit code was not 0, otherwise return bindings
*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, thanks. Done.
@@ -33,51 +33,57 @@ class BindgenReportingSpec extends FunSpec { | |||
.excludePrefix("__") | |||
.generate() | |||
|
|||
result match { | |||
case Right(binding) => assert(binding.errors == errors) | |||
case Left(bindingErrors) => assert(bindingErrors == errors) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should fail test if case Left
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, done
Supersedes #142 (except for the
config
change).