Skip to content
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

feat: improves PHP generation. #7513

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jcchavezs
Copy link

While we can't improve arguments because lack of typing, this is a best effort to improve the code.

Ping @psiinon


if (hasParams) {
out.write(", ");
}
// Always add the API key - we've no way of knowing if it will be required or not
out.write("$" + API.API_KEY_PARAM + "='') {\n");
out.write("string $" + API.API_KEY_PARAM + " = '') {\n");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apikey is always a string.

While we can't improve arguments because lack of typing, this is a best effort to improve the code.

Signed-off-by: José Carlos Chávez <jcchavezs@gmail.com>
@psiinon
Copy link
Member

psiinon commented Oct 10, 2022

The build failed: Run './gradlew :zap:spotlessApply' to fix these violations. 😁

Signed-off-by: José Carlos Chávez <jcchavezs@gmail.com>
Signed-off-by: José Carlos Chávez <jcchavezs@gmail.com>
Copy link
Member

@psiinon psiinon left a 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 no PHP developer 😉

Signed-off-by: José Carlos Chávez <jcchavezs@gmail.com>
@psiinon
Copy link
Member

psiinon commented Oct 11, 2022

Run './gradlew :zap:spotlessApply' to fix these violations. 😁

Signed-off-by: José Carlos Chávez <jcchavezs@gmail.com>
@jcchavezs
Copy link
Author

Fixed @psiinon

@jcchavezs
Copy link
Author

Ping @thc202

out.write(reqParams.toString());
out.write(";\n");
reqParams.replace(0, reqParams.length(), "$params");

for (ApiParameter parameter : optionalParameters) {
String name = parameter.getName();
String varName = name.toLowerCase(Locale.ROOT);
out.write("\t\tif ($" + varName + " !== NULL) {\n");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched tabs into spaces as that is what is being used by standards. In any case, all the code is reformatted by proper PHP linters in the PHP API client.

@@ -34,6 +34,9 @@
import java.util.ResourceBundle;
import java.util.stream.Collectors;

/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

**

Comment on lines +92 to +103
/**
* Generates the API client files of the given API implementors.
*
* @param implementors the implementors
* @throws IOException if an error occurred while generating the APIs.
* @deprecated (2.6.0) Use {@link #generateAPIFiles(List)} instead.
*/
@Deprecated
public void generatePhpFiles(List<ApiImplementor> implementors) throws IOException {
this.generateAPIFiles(implementors);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be needed.

@thc202
Copy link
Member

thc202 commented Oct 21, 2022

Commits should be squashed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants