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

Prevent the error when explicitly specifying default buf gen temlate file path #199

Merged
merged 4 commits into from
Jul 23, 2024

Conversation

un1024
Copy link
Contributor

@un1024 un1024 commented Jun 2, 2024

Backgroud: I encountered the following error.

Buf gen template file specified in the project directory as well as with templateFileLocation; pick one.

But I have just one template file {rootDir}/buf.gen.yaml.
I didn't understand the reason for the error until I read the source code.

I think it's natural to want to specify the file path explicitly.
Therefore how about making this trivial fix to prevent confusion?

Thanks.

@CLAassistant
Copy link

CLAassistant commented Jun 2, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@drice-buf drice-buf left a comment

Choose a reason for hiding this comment

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

LGTM with comments + rebase

@@ -50,8 +50,10 @@ abstract class GenerateTask : DefaultTask() {
check(specifiedTemplateFile != null) {
"Specified templateFileLocation does not exist."
}
check(defaultTemplateFile == null) {
"Buf gen template file specified in the project directory as well as with templateFileLocation; pick one."
if (specifiedTemplateFile != defaultTemplateFile) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can replace the nested if with:

check(defaultTemplateFile == null || specifiedTemplateFile == defaultTemplateFile)

}

buf {
generate {
Copy link
Contributor

Choose a reason for hiding this comment

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

To show that in general it is o.k. to repeat defaults, you could add:

configFileLocation = rootProject.file("./buf.yaml")

and add a single buf.yaml file in this directory like:

version: v1

}

buf {
generate {
Copy link
Contributor

Choose a reason for hiding this comment

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

To show that in general it is o.k. to repeat defaults, you could add:

configFileLocation = rootProject.file("./buf.yaml")

and add a single buf.yaml file in this directory like:

version: v1

@drice-buf
Copy link
Contributor

@un1024 Please let me know in the next day or so if you still want to be involved with this PR, otherwise I will go ahead and push my change to the branch and get the PR merged.

@un1024
Copy link
Contributor Author

un1024 commented Jul 23, 2024

@drice-buf Sorry, I've forgotten this Pull Request and am late... 🙇 I have fixed and pushed for your comments.
Ofcourse I am fine with you to change it there.(If necessary, you can discard my pull request if you like, too.)
Thank you for your time.

@drice-buf
Copy link
Contributor

@un1024 thanks for the PR and for bringing it up to date quickly.

@drice-buf drice-buf merged commit d61f1f9 into bufbuild:main Jul 23, 2024
5 checks passed
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.

3 participants