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

CLI tweaks resolving #250 #332

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

T3sT3ro
Copy link

@T3sT3ro T3sT3ro commented May 26, 2023

Partially implemented #250

  • ferium info should show information about the current profile and modpack
    • ferium profile info added instead to be more scoped
    • ferium modpack info info should be added in a similar manner
  • ferium profile list - tweaked the active profile indicator to be bold green (active) suffix next to profile name.
  • When ferium add fails, link to documentation about overrides (https://github.com/gorilla-devs/ferium#check-overrides) hint about --dont-check-* flags as part of the error message.
  • ferium profile should print the current profile, then Run ferium profile -h for more info about this command (likewise for ferium modpack too) instead use ferium profile info to align with current usage patterns
  • ferium list should print a header with information about the profile about to be listed (i.e. profile name, loader, and mc version) as well
    • added support for --markdown flag in the ferium list -vm variant
  • ferium profile switch should print more info about the profiles, not just the names
    • likewise for ferium modpack switch no need for that
  • ferium profiles should be an alias to ferium profile list (likewise for ferium modpacks too)
  • ferium mods should be an alias to ferium list
  • ferium remove should print the project id of the removed mods too

@theRookieCoder
Copy link
Collaborator

Hmm I don't think a separate info subcommand is needed, I was thinking something like that would be added to the list command instead.

@T3sT3ro
Copy link
Author

T3sT3ro commented May 27, 2023

In the original proposal I mentioned ferium info. I noticed, that ferium profile info is less confusing to get basic info. ferium info poses those questions:

  • Should it print both active profile and active modpack info? Why would I need those two things at the same time? If not, then would I want ferium info for profile info and ferium modpack info for modpack info?
  • How would ferium info be different than ferium list? Is list just a superset of info with an extra mod list?
  • If new project types come into ferium in the future, how would someone get a quick info about each type of projects - modpack, resourcepack, shaderpack, profile? I think the ferium <projectType> info is the most universal and memorable way than having an exception just for mods.

I also noticed, that I often switch profiles and need to know the most crucial info about my active one without the whole long list of mods:

  • MC version
  • loader
  • name

If I were to implement the original proposal of including profile info in the ferium list without the separate ferium profile info, then I would always get more output than I wanted, and couldn't access above information alone:

  • ferium list would print info and (possibly long) mod list to, so I would have to scroll to the top
  • ferium profile list is the only other way to display short profile info, but it also includes other profiles which I again have to look through and possibly scroll.

I noticed that I started using ferium profile info right away - it gives crucial info in a simplest way and I recommend trying it as well.

@T3sT3ro
Copy link
Author

T3sT3ro commented May 28, 2023

I don't know how to add an alias ferium profiles to ferium profile list. and ferium modpacks to ferium modpack list.

I think I finished my job in this PR.

@T3sT3ro T3sT3ro marked this pull request as ready for review May 28, 2023 00:16
@T3sT3ro
Copy link
Author

T3sT3ro commented May 30, 2023

I've noticed, that the download log after ferium update still has the issue of aligning texts using unicode emojis:
image

I noticed it's a problem with padded rust macros e.g. format!({:40}, "<unicode emoji>") where emoji's width is not properly calculated. This is related to rust-lang/rust#30646

@T3sT3ro T3sT3ro changed the title CLI tweaks, partially implement #250 CLI tweaks resolving #250 Jul 6, 2023
@T3sT3ro
Copy link
Author

T3sT3ro commented Jul 6, 2023

I think this can be merged for now as-is after you review @theRookieCoder. If you have some idea how to deal properly with emoji character widths in padded fmt! (and similar) macros, you can adjust them accordingly.

I am a bit hesitant to add profiles alias to profile list, as I imagine it can get later a bit confusing for players, if they for example often use ferium profiles. I imagine they may mistakenly type ferium profiles create ... and it will be prompting them with errors.

@T3sT3ro
Copy link
Author

T3sT3ro commented Oct 29, 2023

@theRookieCoder, will you merge?

@cla-bot
Copy link

cla-bot bot commented Oct 29, 2023

We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please send a signing request to cla-requests@gdlauncher.com and add your github handle to contributors list.

@T3sT3ro
Copy link
Author

T3sT3ro commented Oct 29, 2023

@blarfoon it would be good if you changed cla-bot so it doesn't require sending the email but simply clicking a button/commenting (or use cla-assistant instead).

@blarfoon
Copy link
Member

Yeah eventually you'll just need to click a link and sign the document, this email thing is temporary. Sorry for the inconvenience

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants