-
Notifications
You must be signed in to change notification settings - Fork 155
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
Go: introduce special GoOutputWriter and move generation of error check to it #299
base: master
Are you sure you want to change the base?
Conversation
@@ -15,7 +15,30 @@ sealed trait TranslatorResult | |||
case class ResultString(s: String) extends TranslatorResult | |||
case class ResultLocalVar(n: Int) extends TranslatorResult | |||
|
|||
class GoTranslator(out: StringLanguageOutputWriter, provider: TypeProvider, importList: ImportList) | |||
class GoOutputWriter(indentStr: String) extends StringLanguageOutputWriter(indentStr) { |
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'm very unsure if this is a way to go. Custom OutputWriters are supposed to be used to encompass very special logic: for example, something with very custom formatting (rather than traditional "keep indentation and go line-by-line").
Here, you use it to implement what can be implemented in any Compiler
or Translator
— i.e. pretty much vanilla output by calling puts()
, inc()
, dec()
, etc.
If for some reason keeping it in GoTranslator does not work — let's discuss why and if we should move it into, for example, GoCompiler?
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.
The GoOutputWriter
just add the customization point to intercept attempts to write the error check. Currently KS Translator uses custom writer and heuristics to find and replace the returned error with t.Fatal(err)
call.
This PR allows do the same but in a more elegant way, and also allow to completely skip the error check which is needed for the translator when the error is an expected result.
Probably it is possible to keep this logic in GoTranslator
and make a subclass of the translator in KS Translator to use it here. If that's possible, I'm not sure for what reason that was not done before.
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 agree with @GreyCat that extending StringLanguageOutputWriter
makes little sense in this case from the semantic point of view. I don't see why extending GoTranslator
in the KST translator wouldn't work just as well.
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.
Ok, I rewrited kaitai-io/kaitai_struct_tests#123 so this PR is no longer needed, unless you want to accept a couple of lines of documentation
…ue of type It will be used in KST to check that we return default values when error is expected
db49bf5
to
032007a
Compare
This will allow to override this method and do not generate check in KST when performing checks for returned errors using
asserts[i].exception
key.This should be merged with this PR: kaitai-io/kaitai_struct_tests#123