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

Make intellij plugin aware of proto source dirs #193

Merged
merged 5 commits into from
Dec 5, 2017

Conversation

zpencer
Copy link
Contributor

@zpencer zpencer commented Dec 2, 2017

Verified that with this change, the intellij proto plugin no longer complains about import targets being not found. Jump to definition works as expected in .proto files.

Fixes #156

@zpencer zpencer closed this Dec 2, 2017
@zpencer zpencer deleted the intellij-proto-sources branch December 2, 2017 02:24
@zpencer
Copy link
Contributor Author

zpencer commented Dec 2, 2017

Need to verify that protobuf plugin failing will not impede intellij from opening the project. I need the idea task to run after the protobuf plugin, but it doesn't actually need the plugin to succeed.

@zpencer zpencer reopened this Dec 3, 2017
@zpencer
Copy link
Contributor Author

zpencer commented Dec 3, 2017

@zhangkun83 PTAL

This PR modifies the configuration step of the GenerateProtoTask so that all known proto directories are added to intellij's sourceDir/testSourceDir sets. Because this is the configuration step, we know the proto dirs are registered before the idea task is run, and that the actual success of GenerateProtoTask is irrelevant.

There is a slight gotcha that the idea plugin will silently ignore directories that do not exist. The workaround is to create the directories right before the GenerateIdeaModule task runs. We do not want to create the directory at the configuration phase because it will always run and create the directories, even if we are just trying to do a ./gradlew clean.

Copy link
Collaborator

@zhangkun83 zhangkun83 left a comment

Choose a reason for hiding this comment

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

LGTM except for a minor comment.

f.mkdirs()
}
}
} catch (UnknownDomainObjectException e) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this thrown from getByType()? Since this method is called without checking the presence of the IDEA plugin, this is considered a normal control flow, and exceptions should not be used for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like findByType the non throwing version that I didn't know existed. Updating PR.

@zpencer zpencer force-pushed the intellij-proto-sources branch from 5ca8ecc to 524f010 Compare December 4, 2017 21:54
@zpencer zpencer force-pushed the intellij-proto-sources branch from e6b45d6 to c5dfda8 Compare December 5, 2017 22:39
@zpencer zpencer merged commit 49bd084 into google:master Dec 5, 2017
zhangkun83 pushed a commit to zhangkun83/protobuf-gradle-plugin-1 that referenced this pull request Nov 7, 2018
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.

None yet

2 participants