-
Notifications
You must be signed in to change notification settings - Fork 148
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
chore: Run JDK9Test compiling to validate syntax. #1050
Conversation
@Roiocam I just checked, the paradox task still can't catch the compiling error, but it does when running it locally. |
@mdedetrich @pjfanning @Roiocam I tried to rewrite the plugin which just replaced the |
docs/src/test/java-jdk9-only/jdocs/stream/operators/source/AsSubscriber.java
Outdated
Show resolved
Hide resolved
|
@Roiocam Let's me update the pr and check if that works on ci too. |
I verified both works @Roiocam |
It can now check the error code in Java 9 folder, and for Java 21 I will do in another PR. @mdedetrich @pjfanning |
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.
how does this prove the class has compiled? It doesn't use the class
@pjfanning I mean the file is been compiled in the doc. Check the JDK9+ classes generated is not a target of this PR. Why do you always like to block a PR? you can just leave a comment. |
#1039 isn't even fixed yet - what we really need is to fix #1039 and then come up with a CI check that stops #1039 ever happening again |
@pjfanning I'm preparing a PR for #1039, separately. |
@pjfanning Because you forgot #958 |
It is the duty of the PR writer to explain their PR. You made no mention of #958 and claimed this is to be a fix for #1040 - which it isn't. Can you now explain how this change helps with #958? |
@pjfanning Now it will check the error. as ↑ |
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.
lgtm but I'm going to remove the mention of #1040
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.
LGTM, Thanks for solving this, It should solve #959.
Leave a last picky suggestion.
Co-authored-by: AndyChen <chinatc@outlook.com>
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.
LGTM, thanks.
See #958 - should improve the testing of the doc oriented test code