-
-
Notifications
You must be signed in to change notification settings - Fork 302
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
Added GH actions using tox, pre-commit, black code formatting, flake8 #119
Added GH actions using tox, pre-commit, black code formatting, flake8 #119
Conversation
+ added python related files to gitignore
+ this will allow us to convert the source code into a module to implement GH-Actions
+ project can now be installed using 'python -m pip install -e .'
+ black - code formatting + tests - unit tests in py3.7,3.8,3.9 + pre-commit to verify before push "python -m pip install -r dev_requirements.txt"
else: | ||
if order_status == "cancelled" and float(order[announcement_coin]['_amount']) > float(order[announcement_coin]['_left']) and float(order[announcement_coin]['_left']) > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
black reformatted some lines due to the line length limit of 120.
We should either do the right thing and reduce the amount of indentation for this method or maybe increase the limit to allow these long statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- How should this get fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to increase the limit here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't it be more efficient to extract float(order[announcement_coin]['_left'])
to a separate variable in order to make it more readable? Cause here we call the same var twice and it is what significantly increases length of the string :)
Usually, a length of 120 chars considers as a proper one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly this is not the only case where the lines got split weirdly (see main.py).
I would agree to refactor the code and try to condense it down and keep a max line length of 120.
However I don't wanna add even more complexity to this PR.
My Ideas:
(Option 1) I would refactor these "ugly" lines in future PRs and for now leave it like this (which decreases readability but doesn't add more commits to this PR)
OR
(Option 2) Increase the line length to something like 200 and make the "ugly" lines readable again (in this PR).
I would change it back to 120 in the future and refactor the too long lines then (in individual PRs)
Option 2 would require additional commits here to revert some changes, but keep the codebase afterwards (after merging) more readable (if you consider long lines readable that is).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will go with Option 1 for now and work on it in future PRs, since this is only merged into develop
+ removed python version 3.7 from tox builds + updated default version for pre-commit hooks to python 3.8
This reverts b6723c7
+ Fixed .gitignore to correctly ignore in subdirectories + Changed Dockerfile to install project as module
…trading-bot-binance-announcements-new-coins into feature_github_actions # Conflicts: # Dockerfile # src/main.py
…trading-bot-binance-announcements-new-coins into feature_github_actions # Conflicts: # src/gateio_new_coins_announcements_bot/logger.py # src/gateio_new_coins_announcements_bot/new_listings_scraper.py # src/gateio_new_coins_announcements_bot/send_telegram.py # src/gateio_new_coins_announcements_bot/trade_client.py # src/main.py
I reverted/ignored commit 2340b95 when merging the new changes. The bot should be run from the root directory using the main.py ( |
…trading-bot-binance-announcements-new-coins into feature_github_actions # Conflicts: # src/gateio_new_coins_announcements_bot/new_listings_scraper.py
I added GitHub Actions to automatically run the unit tests (currently 0) and also check code formatting with Flake8.
To get the tests to pass I reformatted all files accordingly.
I also moved the source code into a subdirectory 'src/' and added a 'setup.py'/'setup.cfg' file to package the project as a module. This makes importing of files easier, especially for automated testing.
I also added a pre-commit config so it will automatically validate all files with flake8 on commit and reformat the code using black.
I wrote all necessary instructions into the development_setup.md file.
Some changes i made might cause problems so please take care when code reviewing this. I tried to keep all commits seperate and small, so reviewing changes shouldn't take too long.
Currently mypy doesn't work correctly dues to typing problems. To fix those we need to explicitly define types for variables e.g.
myDict : Dict[str, str] = {}
utilising the 'typing' module.