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

Misleading error message for site commands #1488

Closed
ankush opened this issue Sep 20, 2023 · 6 comments · Fixed by frappe/frappe#25049
Closed

Misleading error message for site commands #1488

ankush opened this issue Sep 20, 2023 · 6 comments · Fixed by frappe/frappe#25049

Comments

@ankush
Copy link
Member

ankush commented Sep 20, 2023

Here the error should be "migrat" command not found, click usually gets typos and suggest right one but our site arg modification is probably messing with it.

λ bench --site dev.localhost migrat
Usage: bench [OPTIONS] COMMAND [ARGS]...
Try 'bench --help' for help.

Error: No such option: --site

Related code:

bench/bench/cli.py

Lines 134 to 138 in dcae3e3

if in_bench:
if cmd_from_sys in get_frappe_commands():
frappe_cmd()
else:
app_cmd()

If we merge frappe and app commands it kind works, but that will likely break other things.

@gavindsouza
Copy link
Collaborator

Something like the following could help probs? (But with additional changes of course - especially replace EXECV usages)

		frappe_commands = get_frappe_commands()
		frappe_command_exists = cmd_from_sys in frappe_commands

		if frappe_command_exists:
			frappe_cmd()

		try:
			app_cmd()
		except Exception:
			from difflib import get_close_matches

			possibilities = get_close_matches(cmd_from_sys, frappe_commands)
			if possibilities:
				raise NoSuchOption(cmd_from_sys, possibilities=possibilities)
			raise

@bosue
Copy link

bosue commented Sep 21, 2023

Related and really awful:

IMG_1572

Aarggh… 🥵😆

edit: Of course, bench --site ap15 run-tests --app lending does work fine. It‘s just not intuitive at all why some flags weirdly need to be set in front of the actual command whereas others have to go to the end.

@ankush
Copy link
Member Author

ankush commented Feb 24, 2024

#1542

This fixes the problem, MILDLY breaking but I doubt anyone uses internal get-frappe-commands via bench API. So eh. 🤷

> bench --site dev.localhost migrat
Error: No such command 'migrat'.

@ankush ankush closed this as completed Feb 24, 2024
@ankush
Copy link
Member Author

ankush commented Feb 24, 2024

Wait, the help message is still misleading... little less than before 😆

Try 'bench frappe --help' for help.

We can probably handle error message in Framework.

@ankush
Copy link
Member Author

ankush commented Feb 24, 2024

image

Overrriding click.Group in Framework works.

@ankush
Copy link
Member Author

ankush commented Mar 4, 2024

@bosue that one is practically unfixable. --site will have to be come first... open to PRs if there's any to fix it.

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

Successfully merging a pull request may close this issue.

3 participants