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

Added GH actions using tox, pre-commit, black code formatting, flake8 #119

Merged
merged 16 commits into from
Jan 3, 2022

Conversation

Linus045
Copy link
Collaborator

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.

+ 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"
setup.cfg Show resolved Hide resolved
main.py Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/black.yml Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
dev_requirements.txt Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
src/gateio_new_coins_announcements_bot/main.py Outdated Show resolved Hide resolved
else:
if order_status == "cancelled" and float(order[announcement_coin]['_amount']) > float(order[announcement_coin]['_left']) and float(order[announcement_coin]['_left']) > 0:
Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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?

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

Copy link
Contributor

@iamtodor iamtodor Dec 30, 2021

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

Copy link
Collaborator Author

@Linus045 Linus045 Dec 30, 2021

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).

Copy link
Collaborator Author

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

src/gateio_new_coins_announcements_bot/main.py Outdated Show resolved Hide resolved
main.py Outdated Show resolved Hide resolved
+ removed python version 3.7 from tox builds
+ updated default version for pre-commit hooks to python 3.8
 + Fixed .gitignore to correctly ignore in subdirectories
+ Changed Dockerfile to install project as module
.gitignore Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
…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
@Linus045
Copy link
Collaborator Author

Linus045 commented Dec 30, 2021

I reverted/ignored commit 2340b95 when merging the new changes.
Therefore the config.yaml and auth.yaml is expected to be in the <root> and <root>/auth directory again instead of <root>/src/.

The bot should be run from the root directory using the main.py (<root>/main.py) also located in the root directory (NOT <root>/src/gateio_new_coins_announcements_bot/main.py).

@Linus045 Linus045 changed the base branch from master to develop January 3, 2022 10:51
…trading-bot-binance-announcements-new-coins into feature_github_actions

# Conflicts:
#	src/gateio_new_coins_announcements_bot/new_listings_scraper.py
@Linus045 Linus045 merged commit 770c81e into CyberPunkMetalHead:develop Jan 3, 2022
@Linus045 Linus045 deleted the feature_github_actions branch January 3, 2022 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants