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

Append source set name for IntelliJ runs classpath module #351

Merged
merged 1 commit into from
May 10, 2017
Merged

Append source set name for IntelliJ runs classpath module #351

merged 1 commit into from
May 10, 2017

Conversation

stephan-gh
Copy link

With these changes ForgeGradle will automatically append the main source set name by default to the module name for the classpath. This is required for the run configurations to properly work in IntelliJ IDEA 2016.1 without additional manual changes.

I've also made the special source set configurable, because some project may use a different source set at runtime than the main one. As an example, SpongeForge has an additional java6 source set which is used to print a proper error to the user to upgrade Java, therefore at runtime the java6 source set is required (which then depends on the main source set). In that case, with this pull request it can be simply configured with:

minecraft {
    runSourceSet = 'java6'
}

I'm still trying to come up with a way to support IntelliJ IDEA 15 at the same time but I haven't found anything simple yet to detect the version the currently created project is using. I'm open for suggestions.

Also make it optionally configurable if projects use a custom sourceset
as main source set
@stephan-gh
Copy link
Author

@AbrarSyed Any chance to merge this? I've searched the whole file intensively, but there is no real easy way to detect the IntelliJ version without reading additional files (to check if a _main module exists), and even then it depends on the project layout you select (e.g. if there is no .idea folder).

There will always be one version you will need to make manual changes with, but in my opinion it should be always the latest release you can use without making any additional changes.

@cpw
Copy link
Contributor

cpw commented Jul 6, 2016

Good to know, I wasn't aware of this. Likely I'll talk to Lex and merge this in.

@kashike
Copy link
Contributor

kashike commented Jul 6, 2016

Should be noted that this PR targets the master branch, while is not the active branch currently, @Minecrell.

@AbrarSyed
Copy link
Member

Someone should move that stuff surround, make 2.2 master and go from there

On July 6, 2016 9:54:21 AM CDT, kashike [email protected] wrote:

Should be noted that this PR targets the master branch, while is not
the active branch currently, @Minecrell.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#351 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@LexManos
Copy link
Member

LexManos commented Jul 6, 2016

As it sits seince we have to keep each version seperate for old versions, I'm liking how each version just gets its own branch and 'master' is dead.

@AbrarSyed
Copy link
Member

I agree with that, but there should be some indication which is 'current', and so far that has been the one on master

On July 6, 2016 2:57:06 PM CDT, LexManos [email protected] wrote:

As it sits seince we have to keep each version seperate for old
versions, I'm liking how each version just gets its own branch and
'master' is dead.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#351 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@Mumfrey
Copy link

Mumfrey commented Jul 6, 2016

Settings -> Branches -> Default Branch

@LexManos
Copy link
Member

LexManos commented Jul 6, 2016

Oh they added that? Done. 2.2

@AbrarSyed
Copy link
Member

Lol, I remember about asking for power to do that ages ago...

On July 6, 2016 3:25:20 PM CDT, LexManos [email protected] wrote:

Oh they added that? Done. 2.2


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#351 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@stephan-gh stephan-gh changed the base branch from master to FG_2.2 August 24, 2016 14:31
@stephan-gh
Copy link
Author

@kashike Changed target branch to FG_2.2 so this could be merged directly now.

@thatsIch
Copy link

thatsIch commented Oct 5, 2016

Is there a reason this is still not merged in?

@MarkL4YG
Copy link

MarkL4YG commented Mar 23, 2017

@thatsIch It appears that it might just happen not to get merged.

@AbrarSyed
Copy link
Member

I guess its been long enough that no one will be using a version of intellij where this wont work anymore..
Somebody test and rebase and all that please, and il merge.

@stephan-gh
Copy link
Author

@AbrarSyed As far I can see it still applies cleanly and I don't see any reason why it would be broken now. Should be fine to merge.

@AbrarSyed
Copy link
Member

@LexManos I approve, take a quick lookover plz

@LexManos LexManos merged commit 7a557d2 into MinecraftForge:FG_2.2 May 10, 2017
@stephan-gh stephan-gh deleted the idea-run-sourcesets branch May 10, 2017 21:08
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.

8 participants