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

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

Merged
merged 54 commits into from
Aug 5, 2022
Merged

Conversation

piiq
Copy link
Contributor

@piiq piiq commented Jul 10, 2022

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.
#1620
#2119
#1634
#1713
#2068
#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 piiq added the bug Fix bug label Jul 10, 2022
@piiq piiq added enhancement Enhancement feat L Large T-Shirt size Feature build Build-related work labels Jul 12, 2022
@piiq piiq changed the title Add helper command to filter unix paths and other edge cases Add parser for input command arguments (The Forward Slash PR) Jul 12, 2022
@piiq piiq self-assigned this Jul 12, 2022
@piiq piiq marked this pull request as ready for review July 12, 2022 14:16
@piiq
Copy link
Contributor Author

piiq commented Jul 18, 2022

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 piiq added the do not merge Label to prevent pull request merge label Jul 19, 2022
@piiq piiq added do not merge Label to prevent pull request merge and removed do not merge Label to prevent pull request merge labels Jul 20, 2022
@piiq piiq removed the do not merge Label to prevent pull request merge label Aug 3, 2022
@piiq
Copy link
Contributor Author

piiq commented Aug 3, 2022

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 piiq merged commit 775d218 into main Aug 5, 2022
@piiq piiq deleted the slash-fix branch August 5, 2022 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fix bug build Build-related work enhancement Enhancement feat L Large T-Shirt size Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants