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

Build out user dir #31

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Build out user dir #31

wants to merge 21 commits into from

Conversation

inverse
Copy link
Contributor

@inverse inverse commented Jul 6, 2021

Prepare for packaging up as an application by moving user data out of the project repository to the user home directories.

This is a significant change.

Partial work for #29

@inverse inverse marked this pull request as ready for review July 8, 2021 21:04
@@ -40,6 +44,49 @@
LONG_INTERVAL_MAP = {v: k for k, v in SHORT_INTERVAL_MAP.items()}


class Paths:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All path based information is encapsulated in this class

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably store this class in another file. With that being said, we should really organize the layout of the project.. Everything is kinda all scattered all across right now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would advise perhaps doing that in another PR. Doing things incrementally and in small commits is easier. Especially to revert if things go south.

algobot/helpers.py Outdated Show resolved Hide resolved
Copy link
Owner

@ZENALC ZENALC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thank you for your contribution my friend!

I will have a more thorough review in the next few days and merge

algobot/__main__.py Outdated Show resolved Hide resolved
@@ -9,7 +9,8 @@
from binance.client import Client
from binance.helpers import interval_to_milliseconds

from algobot.helpers import (ROOT_DIR, get_logger, get_normalized_data,
from algobot import helpers
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here also, why not just have from algobot.helpers import ...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated all

@@ -521,19 +522,20 @@ def get_interval_minutes(self) -> int:
else:
raise ValueError("Invalid interval.", 4)

def create_folders_and_change_path(self, folderName: str):
def create_folders_and_change_path(self, folder_name: str):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice change! We should really convert camel case to snake case over time

Copy link
Contributor Author

@inverse inverse Jul 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

snake case is the python way. If you're happy with something we could use black?

https://black.readthedocs.io/en/stable/

We can configure it as a pre-commit hook even :)

Although not sure how it handles variable naming come to think about it 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pylint will help enforce it ;)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeap. We should really start leveraging pylint and mypy

algobot/helpers.py Outdated Show resolved Hide resolved
algobot/helpers.py Outdated Show resolved Hide resolved
algobot/helpers.py Outdated Show resolved Hide resolved
folder = os.path.basename(targetPath)
os.mkdir(os.path.join(basePath, folder))
if not os.path.exists(target_path):
os.makedirs(target_path)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also use the exist_ok kwarg and set that to True so we don't need to check the conditional

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't that change the semantics of the function?

As in we return true/false if the directory exists?

return True
return False


def open_file_or_folder(targetPath: str):
def open_file_or_folder(target_path: str):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

Copy link
Contributor

@koutsie koutsie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM so far.



def pytest_unconfigure(config):
from algobot.helpers import PATHS, AppDirTemp
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super important this is done here otherwise we init PATHS before setting the env var to control which one gets init

fileName = fileName if fileName != '' else None
backtest_folder_path = helpers.PATHS.get_backtest_results_dir()
create_folder_if_needed(backtest_folder_path)
inner_path = os.path.join(backtest_folder_path, self.backtester.symbol)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably refactor this code to use makedirs()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create_folder_if_needed calls that under the hood.

algobot/helpers.py Outdated Show resolved Hide resolved
algobot/helpers.py Outdated Show resolved Hide resolved
@@ -1,5 +1,6 @@
-r requirements.txt
pytest==6.2.4
pytest-socket==0.4.0
flake8==3.9.0
pre-commit==2.11.1
flake8==3.9.2
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should migrate to pipenv, so much easier with [dev-packages[ and piplock...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do - but lets leave that to another PR ;)

There is also https://python-poetry.org/

I've not used but looks quite nice.

@@ -140,3 +144,17 @@ def test_get_elapsed_time(elapsed, expected):
)
def test_parse_strategy_name(name, expected):
assert parse_strategy_name(name) == expected, f"Expected parsed strategy to be: {expected}."


def test_create_folder_if_needed():
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ZENALC Started adding some tests too but didn't wanna go too overkill yet.

image

Pycharms is reporting this metric

self.infoLabel.setText(f'{directory.capitalize()} files have been successfully deleted.')

if directory == 'Logs': # Reinitialize log folder if old logs were purged.
if directory.endswith('Logs'): # Reinitialize log folder if old logs were purged.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could prob have this defined as a callback and passed into the caller.

But that's for another PR

@ZENALC ZENALC self-assigned this Jul 16, 2021
@ZENALC ZENALC added the enhancement New feature or request label Jul 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants