Skip to content

Switch from getargs to lexopt for arg parsing#302

Draft
IzawGithub wants to merge 3 commits intoethangreen-dev:masterfrom
IzawGithub:getargs
Draft

Switch from getargs to lexopt for arg parsing#302
IzawGithub wants to merge 3 commits intoethangreen-dev:masterfrom
IzawGithub:getargs

Conversation

@IzawGithub
Copy link
Copy Markdown
Contributor

Closes #256

I've only tested on Linux + Proton, I don't have any other machine for that.
Will fix the mac build failure tomorrow.

@WilsontheWolf I see you've said to be platform specific, especially for #198, but I don't see how arg parsing affect symbol resolution?
This feels cleaner for the code to live in core, and that, if platform dependent argument happens, to configure them using a #[cfg] directive.

Also, would like some input on :

https://github.com/IzawGithub/lovely-injector/blob/b66cc765fe12e9d4caacd7be222a8f6b51a15d7c/crates/lovely-core/src/args.rs#L44

Ignoring would fix #212, but I feel like it might be a mistake? In this user case, they probably wanted to pass their custom argument to the executable, not lovely. We could :

  • Do it like how, for example, cargo does it, with the -- --my-arg syntax.
  • Or keep it a warning like how it currently is.

@WilsontheWolf
Copy link
Copy Markdown
Collaborator

@WilsontheWolf I see you've said to be platform specific, especially for #198, but I don't see how arg parsing affect symbol resolution?
This feels cleaner for the code to live in core, and that, if platform dependent argument happens, to configure them using a #[cfg] directive.

Not quite sure the context of what I said was but I was likely talking about #206 (I should rename that PR). Specifically, it's used in a few more obscure environments where setting arguments are harder (such as mobile), or where the tool calling it may want to configure stuff using it's own config tools (such as a config file, or some ui somewhere) and override lovely's handling of it.

https://github.com/IzawGithub/lovely-injector/blob/b66cc765fe12e9d4caacd7be222a8f6b51a15d7c/crates/lovely-core/src/args.rs#L44

Ignoring would fix #212, but I feel like it might be a mistake? In this user case, they probably wanted to pass their custom argument to the executable, not lovely. We could :

Ignoring it would be correct for how we handle things now as we as just a library and not the main executable. Both the executable (love, and the lua code) and lovely get the same args. We don't need to handle passing it or anything. (You can double check in balatro by checking the global arg). I can take a closer look at the code later.


#[derive(Debug)]
pub struct Args {
pub disable_console: bool,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lovely-win will need code adapted to use disable_console (it's the only crate that has a console iirc)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Mhhh, I understand now why you've said args should be living in platform specific crate.
My first idea was to add a target = "windows", but then you need the win library and platform specific dependencies aren't a thing yet in Cargo.

Could be solved by :

  • Doing what I did in the last commit, and make platform specific thing happen in the platform crate. Platform that don't need the arg can simply ignore it. Seems a bit hacky, but work.
  • Or a CommonArg struct that is then embedded in a platform specific args struct. Would need a generic tho

}

let log_dir = mod_dir.join("lovely").join("log");
let args = Args::try_parse().unwrap();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would probably get the sub crate to load the args and then give it to lovely core. This would allow for loading of args via other means if nessicary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

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.

[QUESTION]: Replace getargs with a more modern and mainted arg parser? Lovely panics when parsing external commands

2 participants