Skip to content

Conversation

@kingcavespider1
Copy link
Contributor

Been having trouble getting docker to include the file to locally test this, it builds and passes all tests but almost certainly not done in the best way. I was able to test it outside of the docker image with

./mc-image-helper modrinth --loader forge --projects "paper:simple-voice-chat, fabric:lithium, ferrite-core:beta, datapack:terralith, quilt:comforts:6.3.4+1.20.1" --game-version 1.20.1

It downloaded all successfully

Don't think this is ready to merge without heavy cleanup and testing but could use a second opinion on it

Closes #564

@itzg
Copy link
Owner

itzg commented Jun 9, 2025

Been having trouble getting docker to include the file to locally test this

You might have found this already https://docker-minecraft-server.readthedocs.io/en/latest/misc/contributing/development/#using-development-copy-of-tools but I have to admit it's not easiest testing workflow to get working initially.

Your testing approach with direct command invocation is a lot of what I do anyway during development of mc-image-helper changes.

Copy link
Owner

@itzg itzg left a comment

Choose a reason for hiding this comment

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

Thanks. A couple minor tweaks and it seems good to me.

@kingcavespider1 kingcavespider1 requested a review from itzg June 9, 2025 03:25
@kingcavespider1
Copy link
Contributor Author

Alright that seems like it works for datapacks now

@kingcavespider1 kingcavespider1 requested a review from itzg June 10, 2025 15:39
@itzg
Copy link
Owner

itzg commented Jun 11, 2025

Even though I like most of the whitespace changes, I'm going to revert some of them since it's more important to have well bounded changes per PR. Please wait to push anything until I can push this next commit.

@itzg
Copy link
Owner

itzg commented Jun 11, 2025

Pushed the changes.

Copy link
Owner

@itzg itzg left a comment

Choose a reason for hiding this comment

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

Ready for me to merge? I bet you are 😉

@itzg itzg mentioned this pull request Jun 11, 2025
@kingcavespider1
Copy link
Contributor Author

kingcavespider1 commented Jun 11, 2025

Yeah looks like that worked, was it line ending characters? If so I'm not sure why that happened because it seemed to be working with the previous commits

@kingcavespider1
Copy link
Contributor Author

Whitespace changes were most likely just one of the tools I used deciding it should reformat everything on saving. Usually I'd notice when the diff looks bigger than I expect but gets easier to miss with bigger changes.

@itzg
Copy link
Owner

itzg commented Jun 11, 2025

Yeah looks like that worked, was it line ending characters? If so I'm not sure why that happened because it seemed to be working with the previous commits

Wasn't quite sure in the end since git simply did the right thing as I pushed my changes. Yeah quite odd that it was only the one commit that did that.

@itzg itzg merged commit 0faf93d into itzg:master Jun 11, 2025
1 check passed
@itzg itzg changed the title Prefix system Loader prefix system for Modrinth Jun 11, 2025
@itzg itzg added the enhancement New feature or request label Jun 11, 2025
@itzg
Copy link
Owner

itzg commented Jun 11, 2025

@kingcavespider1 kingcavespider1 deleted the prefix branch June 11, 2025 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support plugin qualifier for Modrinth downloads

2 participants