-
Notifications
You must be signed in to change notification settings - Fork 569
Naive fix for #1413 #1414
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
Naive fix for #1413 #1414
Conversation
@@ -194,4 +195,14 @@ public String getWorkingDirectory() { | |||
public void setWorkingDirectory(@NotNull String workingDirectory) { | |||
myWorkingDirectory = workingDirectory; | |||
} | |||
|
|||
@NotNull | |||
public String getFilePath() { |
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.
Why do you need it in base class? Why do you need duplicates of this methods in GoTestRunConfiguration?
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.
The change to move filePath up to the base is probably unnecessary now. I had originally used this to point to the app root directory until I looked up the source root and eliminated the file path from the run app config entirely. Also, I totally overlooked the TestRun configs so I will fix these tomorrow.
Like I said, totally naive approach and I have never done Idea plugin work before so there are probably much better ways to do this later.
Thank you for help. I see that this is a naive fix, but current implementation is quite naive too and I don't see any reasons fix something in naive way and break other things that works in already implemented naive approach. |
@zolotov Thanks for the comments, and you are absolutely correct. I think the main thing is that to succesfully One thing that I have not experimented with is having multiple Go modules in a single project and seeing if the run configurations will work. I assume this should be supported eventually so I will try to verify soon. I'm really interested to get all of these scenarios working properly because I have projects with all 3 variations of single main() in file, pkg main and main() in application, and the sub directory approach to getting multiple executables. I will give it some more thought as I learn this plugin sdk and see what I can come up with. |
@ccustine thanks, looking forward to your comments. It's really great that you have experience with different workflows in go, because you know most of main contributors are not really go-guys so we have huge lack in understanding workflow questions. About SDK, instead of source roots in this plugin we usually operate with following entities: Also GoSdkUtil contains some handy methods, maybe you'll find them helpful. |
I'm going to close this PR for now. I am working on another approach which should be better and I will just open a new PR because it might be a few days. |
Looking forward to it! |
I have been running this for a few days to get go application run configs to work. I'm sure this could be done much more elegantly and the run configs seem ready for a refactoring but this works for me so I thought it might be useful. I have some other comments for run configs that I will add directly to the thread on #1413.