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

Support for legacyfabric #197

Closed
wants to merge 1 commit into from
Closed

Conversation

BluCobalt
Copy link
Contributor

@Legacy-Fabric ports the fabric toolchain to versions pre 1.14. This pr just adds legacyfabric's meta, using it for versions 1.3.2-1.13.2.

Copy link
Owner

@mindstorm38 mindstorm38 left a comment

Choose a reason for hiding this comment

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

Thank you so much! I didn't know that it existed until now. I have some changes that in my opinion would better fit the stability philosophy of the launcher...

First, I think that this API shouldn't silently hide the normal fabric one (just like we've done with NeoForge), in my opinion we should've a specific function like _with_legacyfabric. Notice the leading underscore, because I would also prefer if these API symbols are unstable -for some time-, to avoid breaking API later if we change the design around legacy fabric support!

This implies that you should also make changes to the CLI (which is allowed to use unstable API because it's internal). Basically adding a search case (https://github.com/mindstorm38/portablemc/blob/main/portablemc/cli/__init__.py#L276) and a start handler (https://github.com/mindstorm38/portablemc/blob/main/portablemc/cli/__init__.py#L448). But also a command line argument to change the version prefix (https://github.com/mindstorm38/portablemc/blob/main/portablemc/cli/parse.py#L51, https://github.com/mindstorm38/portablemc/blob/main/portablemc/cli/parse.py#L147), add it for autocompletion (https://github.com/mindstorm38/portablemc/blob/main/portablemc/cli/parse.py#L162) and add it to search kinds (https://github.com/mindstorm38/portablemc/blob/main/portablemc/cli/parse.py#L216). And finally just add a lang much like for Fabric and Quilt (https://github.com/mindstorm38/portablemc/blob/main/portablemc/cli/lang.py).

Sorry if it feels like a lot more complicated, but I think that it should be quite fast to actually implement, and it allows us to avoid hardcoding supported versions much like you've done wiht LEGACYFABRIC_VERSIONS, which would be complicated to maintain.

Thank you again!

@mindstorm38
Copy link
Owner

mindstorm38 commented Feb 23, 2024

Also, could you retarget your pull request to the v4.3 branch? It is at the same commit as main, so this should be a no-op on your side.

@mindstorm38
Copy link
Owner

Any update on your side? If you need help, do not hesitate, I can just merge this one on the v4.3 branch and do the work myself.

@mindstorm38 mindstorm38 mentioned this pull request Mar 10, 2024
@mindstorm38
Copy link
Owner

If you don't answer by tomorrow I'll merge your work and do the modifications myself.

@BluCobalt
Copy link
Contributor Author

Sorry, I got completely caught up in school and forgot about this. I'd be happy to work on it today.

@mindstorm38
Copy link
Owner

No hurry tho, I thought you really forgot this pr, and I was going to modify it, keeping your work of course.

@BluCobalt
Copy link
Contributor Author

Should I add lang for everything loader specific, or just for legacyfabric_prefix for the help text in parse.py?

@BluCobalt
Copy link
Contributor Author

See #205

@BluCobalt BluCobalt closed this Mar 15, 2024
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.

2 participants