OpenBBTerminal icon indicating copy to clipboard operation
OpenBBTerminal copied to clipboard

[Bug] slash issue

Open andr0id3000 opened this issue 2 years ago • 18 comments

Describe the bug A "bad" load from the econometrics menu will crash the terminal. "Bad" loads have included so far: not giving an alias along with the filename, and setting the filename directly without using the auto-complete feature (which is dope, ty.)

To Reproduce from the terminal, enter the econometrics menu and try to explicitly load a filepath (i used one i just stored from FRED data) - load -f /home/<user>/code/OpenBBTerminal/exports/icsa-test.csv with or without an alias it fails, load: error: argument -f/--file: expected at least one argument which it not accurate, as it was given an argument.

Screenshots Screen Shot 2022-04-03 at 12 06 07 AM

Desktop (please complete the following information):

  • OS: (Host) Mac OS X 12.2.1 running Parallels running (Guest) Ubuntu 20.04.2 ARM64
  • Python version = 3.8.13

Additional context Should be easy to reproduce, hmu in Discord otherwise, happy to help troubleshoot.

andr0id3000 avatar Apr 03 '22 04:04 andr0id3000

@andr0id3000 Looks like an issue here: https://github.com/OpenBB-finance/OpenBBTerminal/blob/5cc2ee9e142837b5d92750c5bd43a82d1419e1c8/openbb_terminal/parent_classes.py#L160-L171

If it finds the / character anywhere in your input it splits your command into seperate commands by / (if I explained it properly)

For example, your command (load -f /home/q/code/OpenBBTerminal/exports/icsa-test.csv) gets changed to:

$ load -f
gives the error
$ home
changes to home
$ q
quit the program

I don't know why it does this, but we should probably change the character used to something like ; so we can enter paths.

You could go into the afformentioned piece of code and change: https://github.com/OpenBB-finance/OpenBBTerminal/blob/5cc2ee9e142837b5d92750c5bd43a82d1419e1c8/openbb_terminal/parent_classes.py#L161

to

        elif ";" in an_input:

and https://github.com/OpenBB-finance/OpenBBTerminal/blob/5cc2ee9e142837b5d92750c5bd43a82d1419e1c8/openbb_terminal/parent_classes.py#L162

to

            actions = an_input.split(";")

I'll open a PR if this fixes it for you.

bolshoytoster avatar Apr 03 '22 15:04 bolshoytoster

For right now the correct way to do this would be to send arguments with a / character in them through quotation marks like this: load -f :/home/<user>/code/OpenBBTerminal/exports/icsa-test.csv". However, I understand this is unintuitive and will get with the team on the best resolution of this. Thanks for the great explanation @bolshoytoster.

colin99d avatar Apr 03 '22 18:04 colin99d

This is exactly right. This is something related with #1599 and it's down to the way we allow the user to create a queue of commands.

This is also how we are able to run routines. We rely strongly on the "slash" separator, thus when a command requires it, this issue occurs.

Examples are exe or star history on the new open source menu. The way we have been doing it is consuming the next command from the queue and updating the queue, which is very hacky and definitely a temporary solution.

I will have a go at a solution for this tonight. I will try to basically create a flag where in certain commands the user will not be able to use the slash operator. This is the only think I can think since it's impossible to allow for both as you wouldn't know when user wanted to use slash for another command or for a path.

DidierRLopes avatar Apr 03 '22 19:04 DidierRLopes

@DidierRLopes you could instead use a different character, I suggested ; because it's commonly used in things like bash already.

bolshoytoster avatar Apr 03 '22 19:04 bolshoytoster

Is there a way to check if the command is an argument if the path does not exist, or to check throw up a warning that says "if there is a / in the comments use quotations. Also, could we look into having maybe using \ and / differently to create a fix (or would this add more confusion).

colin99d avatar Apr 03 '22 20:04 colin99d

@DidierRLopes you could instead use a different character, I suggested ; because it's commonly used in things like bash already.

I think this is the better idea. Has its downsides of course, but will quickly resolve the issue.

colin99d avatar Apr 03 '22 20:04 colin99d

@bolshoytoster this would work, the issue I see here is that it is unnatural for users to do that when specifying a path. Even worst because they can copy-paste quickly a path from their OS and paste it on the platform, in which case that would fail.

@colin99d I think if we were to follow this approach, we would be better to just accept "" instead. because that way at least the Windows users could still copy-paste paths. Thoughts?

DidierRLopes avatar Apr 03 '22 20:04 DidierRLopes

@DidierRLopes sorry, I meant using ; for separating commands - not paths.

bolshoytoster avatar Apr 03 '22 20:04 bolshoytoster

I thought we would keep the file navigation paths normal, and then make navigating through the terminal different. This is a big change tho. A smart way to tell the user to add quotation marks might be better. Maybe we could mark those commands in a certain way so that they are aware

colin99d avatar Apr 03 '22 20:04 colin99d

Wait, since commands should never have navigation after them there has to a way to just create logic for this

colin99d avatar Apr 03 '22 20:04 colin99d

Ohhh @bolshoytoster I see!! I like that!! It's not as visible as "/" but I think is less confusing in the long run.

@colin99d ?

DidierRLopes avatar Apr 03 '22 20:04 DidierRLopes

I was originally thinking the same thing as @bolshoytoster, but do you think we can just build logic into the parser. Because commands will never have navigation after them.

colin99d avatar Apr 03 '22 20:04 colin99d

@colin99d what do you mean by navigation after them?

DidierRLopes avatar Apr 03 '22 20:04 DidierRLopes

@bolshoytoster we love the idea! Were you wanting to do the PR for this?

colin99d avatar Apr 03 '22 20:04 colin99d

@colin99d I could do.

bolshoytoster avatar Apr 03 '22 20:04 bolshoytoster

@bolshoytoster I look forward to reviewing it!

colin99d avatar Apr 03 '22 20:04 colin99d

@DidierRLopes

We rely strongly on the "slash" separator

Could you direct me to where it's used please, I tried using a regex search but it gave 1712 maches and I don't quite fancy going through that many.

bolshoytoster avatar Apr 03 '22 21:04 bolshoytoster

Yeaaa, I figured. Don't worry with it, I think me and @colin99d can handle this from our side.

I think the main things to fix are:

  • parents_classes
  • scripts folders
  • routines folders
  • call_exe and call_sh funds that have the hacky version to parse the "/" - there's prob 1 more functionality like this

DidierRLopes avatar Apr 03 '22 23:04 DidierRLopes

This will be checked on next major release.

Chavithra avatar Aug 26 '22 08:08 Chavithra