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

Adds option to add hemtt parameters to launch config #739

Closed
wants to merge 9 commits into from

Conversation

Crowdedlight
Copy link
Contributor

Questions:

  1. Currently that function seems to fail either if you set the same parameter in both the config and cli (hemtt launch nodebug --quick), or if you use an option that doesn't exist, like --quack. I assume we do not want it to fail and error out if the same option is defined on both CLI and in the config, but "remove duplicates" and carry on?
  2. Wasn't sure to what degree I should make the error message. It could be neat to make it show or print which specific entry in the options was not found, but I might need a pointer on how to build errors of that type and how to detect which one that fails.
  3. wasn't sure on the config entry name. Not sure if just "options" would be better?

Tested:
Tested on my arma3 mod with this launch config, and seemed to work.

[hemtt.launch.nodebug]
workshop = [
    "450814997",  # CBA_A3's Workshop ID
    "894678801",  # TFAR Beta
    "1779063631", # Zeus Enhanced 
    "2369477168", # Advanced Developer Tools
    "463939057",  # ACE
    #"1645973522", # Intercept Minimal Dev
    #"1585582292", # Arma Debug Engine
]
mission = "dev_mini.Enoch" # Launch into existing Editor Mission
parameters = [
]
cli_options = [
  "--no-filepatching",
  "--expsqfc",
  "--quick",
]

Current Error message if option given is not valid:
billede

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

Attention: Patch coverage is 12.50000% with 42 lines in your changes missing coverage. Please review.

Project coverage is 68.6%. Comparing base (21b2b2e) to head (9855185).

Files Patch % Lines
...unch/error/bcle10_launch_config_wrong_parameter.rs 0.0% 21 Missing ⚠️
bin/src/commands/launch/mod.rs 0.0% 17 Missing ⚠️
libs/common/src/config/project/hemtt/launch.rs 50.0% 4 Missing ⚠️
Additional details and impacted files
Files Coverage Δ
bin/src/commands/dev.rs 88.4% <100.0%> (+<0.1%) ⬆️
libs/common/src/config/project/hemtt/launch.rs 76.4% <50.0%> (-3.9%) ⬇️
bin/src/commands/launch/mod.rs 19.0% <0.0%> (-0.9%) ⬇️
...unch/error/bcle10_launch_config_wrong_parameter.rs 0.0% <0.0%> (ø)

... and 4 files with indirect coverage changes

Copy link
Owner

@BrettMayson BrettMayson left a comment

Choose a reason for hiding this comment

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

I think we want to check that every option starts with --, to prevent the use of short flags, and to prevent the sort breaking things like

cli_options = [
  "--optional",
  "some_optional",
]

where instead it should be used as

cli_options = [
  "--optional=some_optional"
]

bin/src/commands/launch/mod.rs Outdated Show resolved Hide resolved
@Crowdedlight
Copy link
Contributor Author

  • Changed to get_matches_from() as suggested for the error handling.
  • Added error message for cases where the cli_options in config doesn't start with --
  • Added cli_options to the book under the launch page

Still seems to have some trouble overriding options with default values. Like --instances=2 does not seem to update the default value set in ArgMatches. (I know I would have to update the instance_count as that is set before the config is loaded, but during testing I tried to update it after loading the config and also inspecting the ArgMatches afterwards just shows instances set to 1 as default.

I have not been able to validate that --optional=some_optional syntax actually works as I don't have any mods with optionals ready, but I do split it into ["--optional", "some_optional"] as clap has it in their documentation.

I can not get --threads to work either, neither when set in launch config or just passed on cli. It returns

hemtt launch --threads 8
error: unexpected argument '--threads' found

This does not happen on main, so my changes have caused --threads to not be registered anymore. I think it might be because let matches = cli().get_matches_from(...) is called on cli which does not have threads defined, as I assume that is given somewhere else?

@BrettMayson
Copy link
Owner

Would it have made more sense to just add more options to the launch config?

After removing the ones that aren't compatible or are already supported by the launch config, it's really just like quick, no-rap, and no-filepatching that are missing. It would've been simpler to just add those to the launch config

@Crowdedlight
Copy link
Contributor Author

Would it have made more sense to just add more options to the launch config?

After removing the ones that aren't compatible or are already supported by the launch config, it's really just like quick, no-rap, and no-filepatching that are missing. It would've been simpler to just add those to the launch config

That is a good point. Makes it more simple!

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