OpenBBTerminal icon indicating copy to clipboard operation
OpenBBTerminal copied to clipboard

Add parser for input command arguments (The Forward Slash PR)

Open piiq opened this issue 1 year ago • 3 comments

Problem:

Our input parsing behaves incorrectly when there is a forward slash in the command queue. It is known to affect unix style paths and sorting arguments for screeners.

There are issues reporting this and PRs proposing fixes. https://github.com/OpenBB-finance/OpenBBTerminal/issues/1620 https://github.com/OpenBB-finance/OpenBBTerminal/pull/1634 https://github.com/OpenBB-finance/OpenBBTerminal/pull/1713 https://github.com/OpenBB-finance/OpenBBTerminal/pull/2068 https://github.com/OpenBB-finance/OpenBBTerminal/pull/1974

Solution:

This PR introduces a filtering function that searches for paths and special arguments and replaces them with placeholders for the time it is splitting the input into individual commands.

This should resolve the problem of unix paths or github repo names to be considered individual commands.

Along with the fix this PR introduces the following rules:

  1. The -f argument is reserved for file paths
  2. When loading data from a file by providing a full path the -f or --file argument is mandatory

load /mydir/myfile.xls/ta/ema <- will NOT work load -f /mydir/myfile.xls/ta/ema <- will work

Custom per-controller argument filters allow parsing edge cases in specific controllers. For example in alternative/oss this: sh openbb-finance/openbbterminal/../covid <- will work

Linting: To support the rules a linter is added to the pre-commit hooks that checks that the short reserved command arguments are used with correct long command arguments

Additional fixes:

  • fixed portfolio model tests
  • fixed finviz screener tests

piiq avatar Jul 10 '22 22:07 piiq

There was a problem with tests related to dep version mismatch in the runner envs that forced recreating the test data. Sorry for that 🤷‍♂️

piiq avatar Jul 12 '22 14:07 piiq

I'm going to take some time to review the code to provide additional comments but I wanted to start a discussion on the approach in general first.

OpenBB takes the opinionated stance that / is the single way to separate commands. This means all commands must acknowledge this and anything that requires forward slashes must respect this and accommodate it, such as using the new -f syntax. Examples are filepaths and fractions, though their could be others in the future. It is fine to be opinionated.

My question is why is this the opinion that is held for the best way to separate commands? Shells (like Bash, TCL, etc) typically use semicolons, &&, or || to separate commands so it is surprising to me that forward slashes are used. My personal opinion is that using semicolons such as that proposed in #1634 would be simpler and wouldn't require as much special processing for things as common as filepaths. I would wager many people coming to OpenBB would expect something like a semicolon to be used and would be surprised that a forward slash is the command separator (that was my personal experience when I discovered it.)

stkerr avatar Jul 18 '22 18:07 stkerr

Thanks for the question @stkerr

Back in the days (a year ago 😂) we had a lengthy discussion on the separator and the slash was choses because the contexts/menus structure resembles the folder tree. The use case would be to jump into contexts and menus from the root of the terminal as if calling a command from a nested folder in a shell. /stocks/disc/gainers would get you to the gainers command in the disc "folder" in the stocks "folder". And a single / takes to the terminal root.

2022 Jul 18, 16:16 (🦋) /stocks/disc/ $ gainers

The options that we had on the table were slash /, semicolon ; and the pipe symbol | and after a discussion on discord and a vote we sticked to the slash for simplicity. At that time we didn't support importing or exporting data to arbitrary paths and the controllers looked different to what we have right now. So the slash has got entrenched in the functionality of the controllers, the routines we write to test and use the terminal and the docs.

Having a custom arbitrary separator (; or && or anything else) is totally possible. Nowadays most of the terminal-wide customizations are done through feature flags. Do you think the command separator should be customizable like that? I'm open to jump on a call on discord to discuss a customization PR

piiq avatar Jul 18 '22 20:07 piiq

Hey folks! I've added a few fixes and updated this PR so it's ready to merge once the tests pass.

After the merge it would be great to look into enabling loading of portfolio files from arbitrary paths #1974

piiq avatar Aug 03 '22 20:08 piiq