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

A new CodeGenerator.createFile method with more flexibility #1041

Merged
merged 14 commits into from
Aug 12, 2022

Conversation

jameskleeh
Copy link
Contributor

Problem:
The current API assumes classes are being created by enforcing package name and extension arguments. Many use cases for creating files include creating resources. This requires passing in a dummy "package name" and an empty extension in many cases. The processing of the package name argument assumes that dots should be replaced with slashes to represent separate directories. This prevents the creation of directories with dots in their name.

Solution:
This PR maintains the previous behavior with the existing API and introduces a new method to allow users more flexibility in the path being created. Suggestions welcome

Copy link
Contributor

@neetopia neetopia left a comment

Choose a reason for hiding this comment

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

Overall this looks good, but I would refrain from introducing a new FileType enum, this is helpful but will make the 2 createNewFile() apis inconsistent in terms of the way they handle file types. If necessary we can introduce FileType later in a uniformed way for all APIs, for now just checking the extension name should help, unless we have a use case for example, generating .java files into resource directory?

@jameskleeh
Copy link
Contributor Author

jameskleeh commented Jul 14, 2022

@neetopia Having to pass in an extension as a mechanism to choose a directory doesn't make sense in the cases where there is no file extension. In addition, if the fileType was changed to extensionName: String, wouldn't that clash with the existing API where the last argument has a default value?

In addition the translation between the extension and destination directory isn't obvious

@neetopia
Copy link
Contributor

I am not completely against the idea of FileType, but do not want to make it consistent for 2 file creation APIs, and change the existing one will be a breaking change therefore we want to evaluate it later to see if we want to go that approach.

That being said, even if we want to use FileType for the existing file creation API, it will still cause same signature clash issue, to avoid signature clash, an option is to name the function differently.

@jameskleeh
Copy link
Contributor Author

@neetopia I've made the changes we discussed on slack

Copy link
Contributor

@neetopia neetopia left a comment

Choose a reason for hiding this comment

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

Can you please fix the lint error and the failing test?

* Used to determine where files should be stored when being
* created through the [CodeGenerator].
*/
enum class FileType {
Copy link
Contributor

Choose a reason for hiding this comment

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

This FileType should be unnecessary then? I see that you do have usages in your code relying on this enum, but since we do not have any APIs exposing this enum, maybe we can put it in our implementations, if this enum helps with your implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, got rid of it

@jameskleeh
Copy link
Contributor Author

@neetopia Tests are passing

@neetopia
Copy link
Contributor

you can run ./gradlew ktlint to see the failure on lint.

@jameskleeh
Copy link
Contributor Author

@neetopia The lint issue is resolved

Copy link
Contributor

@neetopia neetopia left a comment

Choose a reason for hiding this comment

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

LGTM, please fix the api compatibility check so I can merge this PR.

@neetopia neetopia merged commit 1b5864d into google:main Aug 12, 2022
@neetopia
Copy link
Contributor

Merged, thanks for your PR!

github-actions bot pushed a commit that referenced this pull request Aug 12, 2022
* Support creating files with a more flexible API

* Change associate api to use extension

(cherry picked from commit 1b5864d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants