This repository has been archived by the owner on Aug 20, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 176
Let firrtl based applications run despite loading unknown annotations #2387
Merged
Merged
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
3cd4288
Let firrtl based applications run despite loading unknown annotations
chick 9232a2b
Merge branch 'master' into allow-unrecognized-annotations
chick f5aa131
A number of fixes in response to PR comments
chick 97f25ca
- changed suppressed back to ignored
chick 6759ec7
- cleaned up annotation test data for UnrecognizedAnnotationSpec
chick 9911e73
- scalafmt
chick 8bccb9a
- moved regex to object level
chick 4e7ee80
- scalafmt
chick 8dda907
- fixed missing distinct in building list of unrecognized annotations
chick 8007d54
- JsonProtocol
chick f410b9d
Merge branch 'master' into allow-unrecognized-annotations
chick f8b0b7e
- JsonProtocol
chick dfe6e65
Merge branch 'master' into allow-unrecognized-annotations
jackkoenig 2faf904
Merge branch 'master' into allow-unrecognized-annotations
mergify[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -564,7 +564,7 @@ class JsonAnnotationTests extends AnnotationTests { | |
val manager = setupManager(Some(anno)) | ||
|
||
the[Exception] thrownBy Driver.execute(manager) should matchPattern { | ||
case InvalidAnnotationFileException(_, _: AnnotationClassNotFoundException) => | ||
case InvalidAnnotationFileException(_, _: UnrecogizedAnnotationsException) => | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect, but am not certain, that this change will change the error you receive if you have an unserializable annotation (eg. an annotation that just wraps a random non-case class). Can you add a test here showing what happens in that case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the second to last test in AnnotationTests demonstrate this? |
||
|
||
|
@@ -614,4 +614,37 @@ class JsonAnnotationTests extends AnnotationTests { | |
val cr = DoNothingTransform.runTransform(CircuitState(parse(input), ChirrtlForm, annos)) | ||
cr.annotations.toSeq shouldEqual annos | ||
} | ||
|
||
"fully qualified class name that is undeserializable" should "give an invalid json exception" in { | ||
val anno = """ | ||
|[ | ||
| { | ||
| "class":"firrtlTests.MyUnserAnno", | ||
| "box":"7" | ||
| } | ||
|] """.stripMargin | ||
|
||
val manager = setupManager(Some(anno)) | ||
the[Exception] thrownBy Driver.execute(manager) should matchPattern { | ||
case InvalidAnnotationFileException(_, _: InvalidAnnotationJSONException) => | ||
} | ||
} | ||
|
||
"unqualified class name" should "give an unrecognized annotation exception" in { | ||
val anno = """ | ||
|[ | ||
| { | ||
| "class":"MyUnserAnno" | ||
| "box":"7" | ||
| } | ||
|] """.stripMargin | ||
val manager = setupManager(Some(anno)) | ||
the[Exception] thrownBy Driver.execute(manager) should matchPattern { | ||
case InvalidAnnotationFileException(_, _: UnrecogizedAnnotationsException) => | ||
} | ||
} | ||
} | ||
|
||
/* These are used by the last two tests. It is outside the main test to keep the qualified name simpler*/ | ||
class UnserBox(val x: Int) | ||
case class MyUnserAnno(box: UnserBox) extends NoTargetAnnotation |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
can this have scaladoc added?
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 could but shouldn't an unrelated tests like this go in a separate PR?
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.
@chick I think you may have responded to the wrong comment
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.
@jackkoenig @mwachs5 I meant to say, adding scaladoc to pre-existing code that is unrelated to this PR should go on a separate PR. But I'm happy to add some. Any thoughts on what it should say, I feel like DeletedAnnotations are heading towards deprecation and using them, particularly looking inside them, is an anti-pattern