Allow loading of portfolio from arbitrary paths

[stkerr]

Description

This PR allows for loading of files that are located anywhere on a user's system, rather than just those limited to the pre-determined load path.

It also adds the default load path to the help message to help the user understand where files will be loaded from first.

• [x] Summary of the change / bug fix.
• [x] Link # issue, if applicable.
• [x] Screenshot of the feature or the bug before/after fix, if applicable.
• [x] Relevant motivation and context.
• [x] List any dependencies that are required for this change.

How has this been tested?

Tried local test workflows several times successfully. Pytest all passes.

Checklist:

• [x] Update our Hugo documentation following these guidelines.
• [x] Update our tests following these guidelines.
• [x] Make sure you are following our CONTRIBUTING guidelines.
• [x] If a feature was added make sure to add it to the corresponding scripts file.

Others

• [x] I have performed a self-review of my own code.
• [x] I have commented my code, particularly in hard-to-understand areas.
• [x] My code passes all the checks pylint, flake8, black, ... To speed up development you should run pre-commit install.
• [x] New and existing unit tests pass locally with my changes. You can test this locally using pytest tests/....

[stkerr]

@jmaslek @DidierRLopes can you review this PR? Thanks!

[stkerr]

@DidierRLopes thanks for taking a look.

I don't think using default here for argparse will work. This check_file_existence function needs two parameters to work, the filename to load and the path to search for. We can set a default value for the path in argparse, but there isn't a reasonable value to set for the default filename. If we set a default, the function will get the path value only and not have a filename.

It's possible to hardcode the value of the self.DEFAULT_HOLDINGS_PATH path in the function, but at that point the function is tied directly to the argparse code, so should really just be in the call_load function rather than standalone.

An alternative would be if we always looked at os.getcwd() by default (my preference), but that's a behavior change so not sure if you're open to that.

Thoughts?

[DidierRLopes]

I think this may be what you are looking for:

The os.getcwd() I think could get messy really fast by having all files dropped at the root of where program is executed.

For now what you add is already an improvement, so I would be happy to merge. The only thing I may ask is if you can add this for Mac as well. Because of the paths in Mac using forward slash this breaks with the "queue" that we have set at OpenBB - we are currently working on a fix for this.

Something as easy as this should work (probably make sure that forward slash exists when provided),

        parser.add_argument(
            "-f",
            "--folder",
            type=str,
            dest="folder",
            help="Folder where to export data. 'default' redirects to OpenBB Terminal 'exports'",
            default="default",
        )
        if other_args and "-" not in other_args[0][0]:
            other_args.insert(0, "-f")
        ns_parser = parse_simple_args(parser, other_args)

        if ns_parser:
            if other_args or self.queue:
                if other_args:
                    export_path = ""
                else:
                    # Re-add the initial slash for an absolute directory provided
                    export_path = "/"

                export_path += "/".join([ns_parser.folder] + self.queue)
                self.queue = []

[stkerr]

@DidierRLopes ahh ok, I see what you're saying about the lambda now. I moved the logic into a function that gets returned to replace the lambda.

For the Mac topic, I don't have a Mac to test on but do have a Linux box, which should be ok since it uses forward slashes for paths as well. Let me know if there is something Mac specific that wouldn't show up on Linux though. I think I see what you mean about the queue too - I'll see if I can figure that out!

Linux:

Windows:

[DidierRLopes]

Thanks @stkerr !

Just ping me when this is ready to be reviewed and I can test it 😄

[stkerr]

@DidierRLopes I've been trying to think of ways to address the feedback about Unix paths and not sure how to proceed.

The difficulty I'm running into is that the user input gets split into chunks based on / before it ever reaches the call_load function. See the picture below:

This means that there isn't going to be a way to add logic into to call_load function as far as I can tell to make it respect Unix filepaths, since the input is all chopped up before it ever receives it. In the example above, I called the function with a full path but the function only received [..] as its parameter.

I'm thinking there are a few ways to proceed and looking for your input:

• Commit it as is. This enables arbitrary file paths for Windows but not Unix systems. Unix systems will be able to now use files from the current working directory though, which is a plus.
• Update the call_load function to use \ characters for all file paths, including Unix systems. This will feel awkward on Unix systems, but would work.
• Update the input parsing logic further upstream to not chop up the input depending on the desired command(s). I suspect this would be difficult and is beyond what I'm comfortable doing.

[DidierRLopes]

Hey @stkerr,

Yes, that is an issue that we are currently aware of. There are 2 PRs that currently have a POOC that addresses this, and we are discussing internally the best solution to move forward.

We have been avoiding this using an "hack". Basically we just take the queue and concatenate back to the path, the issue is that if there's a file path followed by a command that will be considered all the file path. See for example alternative/oss/sh.

FYI @piiq @Chavithra we should bump this up on the priority queue.

[DidierRLopes]

Hey @stkerr,

Can you check this https://github.com/OpenBB-finance/OpenBBTerminal/pull/2064 and give your feedback please?

@piiq had a crack at the forward slash issue

[stkerr]

@DidierRLopes will do! Going to write a comment with my thoughts on that PR.

[DidierRLopes]

Hey @stkerr ,

The team has internally decided to go forward with the forward slash. Is this PR still relevant?

Cheers!

[montezdesousa]

I will close this PR, seems to be fixed by #2091.