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

Suppress "--add_dll_search_path" in sub-parser #1737

Merged
merged 1 commit into from
Dec 31, 2024

Conversation

jere8184
Copy link
Contributor

No description provided.

@jere8184 jere8184 force-pushed the suppress_add_dll_search_path branch 8 times, most recently from 32c138d to 1525fac Compare December 29, 2024 20:49
@jere8184 jere8184 changed the title Suppress add dll search path Suppress "--add_dll_search_path" in sub-parser Dec 29, 2024
@TheJJ
Copy link
Member

TheJJ commented Dec 29, 2024

why is this change needed? the parameter is added to the toplevel cli on windows only already?

@jere8184
Copy link
Contributor Author

why is this change needed? the parameter is added to the toplevel cli on windows only already?

if you fail to provide a subcommand this parameter will be passed to the 'main' sub-parser causing it to fail with an error.
run locally (on windows)
'python -m openage --add-dll-search-path <valid_path>'
and see for yourself.

you should get the following error
'openage main: error: unrecognized arguments: --add-dll-search-path <valid_path>'
which is pretty confusing.

This change stops an error from occurring and invokes 'openage main' as if no arguments have been passed.

@TheJJ
Copy link
Member

TheJJ commented Dec 30, 2024

ah, then the proper solution is to add this as a common parent to the subparsers that make use of the option. duplication is not the proper way, unfortunately.

@jere8184 jere8184 force-pushed the suppress_add_dll_search_path branch from 1525fac to 0a03e2d Compare December 31, 2024 13:18
@jere8184
Copy link
Contributor Author

@TheJJ i've went a slightly different, route see changes.

Copy link
Member

@heinezen heinezen left a comment

Choose a reason for hiding this comment

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

Looks good to me

@heinezen heinezen merged commit b35c9bf into SFTtech:master Dec 31, 2024
13 checks passed
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.

3 participants