-
Notifications
You must be signed in to change notification settings - Fork 0
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
BUPH-21 | add more Annotation Processor documentation #32
Conversation
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.
Looks good! Just one clarification suggestion.
``` | ||
- `AnnotationProcessorKey`: Also used in the fabrication file to tie the annotation to the specific AP. | ||
- `Default Contents`: If no AnnotationProcessor is specified in the fabrication file, this is used instead | ||
- There MUST be a newline between the `AnnotationProcessorKey` and `Default Contents` |
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.
It may be worth being more explicit here that if there are no Default Contents
, the end of comment identifier (*/
) must still be on a new line.
Eg:
Correct:
/** @neighborhoods-buphalo:annotation-processor Builder.setters
*/
Incorrect:
/** @neighborhoods-buphalo:annotation-processor Builder.setters */
While this line technically covers this case, (as I mentioned in my rescinded approval) I have lost an afternoon to this problem because the incorrect example works with one AP in a template, but once you add a second AP, things break.
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 thought that was the case, but I just tested it and got it to work if I didn't include Default Contents. It may have been fixed since then?
public const TEST_STRING = "/** @neighborhoods-buphalo:annotation-processor TestSimpleStringAnnotationProcessor */";
plus
TestSimpleStringAnnotationProcessor:
processor_fqcn: \Neighborhoods\Buphalo\V1\AnnotationProcessors\SimpleString
static_context_record:
string: 'this is a string'
produced
public const TEST_STRING = "this is a string";
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 problem actually comes up when you have more than 1 annotation processor in a file. Assuming Buphalo uses that same regex matching as Bradfab, I believe the below example will break. The first AP will replace everything from the first /**
after TEST_STRING = "
to the last */
after ThisWillProbablyDisappearToo
.
public const TEST_STRING = "/** @neighborhoods-buphalo:annotation-processor TestSimpleStringAnnotationProcessor */";
// This will probably disappear
/** @neighborhoods-buphalo:annotation-processor ThisWillProbablyDisappearToo */
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.
because the incorrect example works with one AP in a template, but once you add a second AP, things break.
Huh, just re-read this, and that's very interesting. I'll see if I can replicate with some tests.
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.
Thanks, @jakemalachowski
I've been able to validate this with tests. Created BUPH-92 | Single-Line Annotation Tags Don't Work Well With Others and #38 for us to address.
Also adding @jakemalachowski for some external feedback since he's used them before.