OpenBB
OpenBB copied to clipboard
Add parser for input command arguments (The Forward Slash PR)
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:
- The
-f
argument is reserved for file paths - 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
There was a problem with tests related to dep version mismatch in the runner envs that forced recreating the test data. Sorry for that 🤷♂️
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.)
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
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