-
Notifications
You must be signed in to change notification settings - Fork 199
Fabric support #733
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
Fabric support #733
Conversation
src/main/resources/fileTemplates/j2ee/fabric/fabric_mixins.json.html
Outdated
Show resolved
Hide resolved
…n.html Co-authored-by: comp500 <comp500@users.noreply.github.com>
RedNesto
left a comment
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.
Make sure to catch IOExceptions when instanciating JarFiles, this is the only real issue I found in this PR. There's also a TODO that's not been taken care of (I guess?), the rest is pretty much formatting/styling comments.
I'll test it soon, but overall it looks good.
src/main/kotlin/com/demonwav/mcdev/platform/fabric/FabricModule.kt
Outdated
Show resolved
Hide resolved
| StandardOpenOption.CREATE, | ||
| StandardOpenOption.WRITE, | ||
| StandardOpenOption.TRUNCATE_EXISTING |
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.
These options are redundant, they can be removed. From the method's javadoc:
If no options are present then this method works as if the CREATE, TRUNCATE_EXISTING, and WRITE options are present.
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.
I copied that from CreatorStep.writeTextToFile, and those options are present explicitly in many places throughout the codebase. Someone should go through and clean that up really.
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.
Neat, I didn't realize those were the defaults. That's good to know.
src/main/kotlin/com/demonwav/mcdev/platform/fabric/creator/FabricProjectCreator.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/com/demonwav/mcdev/platform/fabric/creator/FabricProjectSettingsWizard.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/com/demonwav/mcdev/platform/fabric/framework/FabricPresentationProvider.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/com/demonwav/mcdev/platform/fabric/inspection/UnresolvedReferenceInspection.kt
Outdated
Show resolved
Hide resolved
|
I had to change a little how the semver parser works. Fabric converts Minecraft versions to semver e.g. with |
|
If this gets approved, maybe you could change the README.md and the Website accordingly too? Aka add Fabric to the supported Minecraft Development Platforms |
|
Oh right, good point |
|
When is this going to get merged? |
|
Awesome! It'd be great if this were merged soon so we don't need to switch between plugin versions every time we need to use Fabric. |
|
Took a bit of a break, sorry. Everything looks sane to me, nothing sticks out that I think really needs to be changed. |
This PR adds Fabric support. Along with simply porting what was on my fork to the new project creator refactor, I decided to go a little further and add the following features which are not on my fork:
Closes #663