-
Notifications
You must be signed in to change notification settings - Fork 7
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
Xray integration test #113
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Please refactor with this naming convention instead
@@ -429,4 +461,26 @@ private static class GrammarParseException extends RuntimeException { | |||
+ "%nSee the below error message for more details", originalSource, title), e); | |||
} | |||
} | |||
|
|||
public interface PickleJarFactoryGenerator extends QuadFunction<PdslGherkinRecognizer, |
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 don't think we're actually using this or the QuadFunction anywhere. Can you delete them if this is the case?
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.
removed
@@ -57,19 +57,19 @@ public DefaultPdslTestCase(String testCaseTitle, List<TestBodyFragment> testBody | |||
} | |||
|
|||
@Override | |||
public URI getOriginalSource() { return source; } |
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.
These are not backwards compatible changes!
If you modify a public API someone else may break someone else's code!
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 automatically comes up.. i didn't do this change
import java.util.Properties; | ||
import java.util.logging.Logger; | ||
|
||
public class XrayAuth { |
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.
XRay doesn't belong in the standard library!
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.
moved
private final String clientId; | ||
private final String clientSecret; | ||
|
||
public XrayAuth(String xrayUrl, String clientId, String clientSecret) { |
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.
You need Javadocs on all public classes and methods
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.
added javadocs
import java.util.Iterator; | ||
import java.util.List; | ||
|
||
public record XrayTestCase(URI originalSource, List<String> unfilteredPhraseBody, |
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.
Javadocs, no XRay classes in standard library
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.
moved
src/main/resources/xray.properties
Outdated
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.
Get rid of XRay properties in the standard library.
Also....
WHAT THE HECK WERE YOU THINKING PUTTING SECRETS IN A PUBLIC SPACE!!!! PULL IT DOWN! PULL IT DOWN!!!!!!!!!!
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.
removed
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.
This looks like a test specifically for XRay. It's not suitable for the standard library.
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.
removed
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.
No
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.
moved
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.
Remove your backups from all of your CLs.
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.
removed
No description provided.