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

Implement github actions #25

Merged
merged 11 commits into from
Mar 1, 2021
Merged

Implement github actions #25

merged 11 commits into from
Mar 1, 2021

Conversation

SabineHaas
Copy link
Collaborator

@SabineHaas SabineHaas commented Feb 18, 2021

Fix #6

Changes proposed in this PR:

  • Add github actions workflow including:
    • run tests with pytest
    • add test coverage
    • check linting with black
  • Remove test_requirements.txt and add extras_require to setup.py

The checks are done for python 3.7 and 3.8 currently, this can be adapted of course.

@SabineHaas SabineHaas self-assigned this Feb 18, 2021
@SabineHaas SabineHaas marked this pull request as ready for review February 18, 2021 09:17
@SabineHaas SabineHaas requested a review from jnnr February 18, 2021 09:50
@SabineHaas
Copy link
Collaborator Author

SabineHaas commented Feb 18, 2021

@jnnr and @juliusmeier you can lint your code with Black (run black . --exclude docs/). The CI will remind you to do so, as the build will fail if the code is not linted with black, as soon as #25 is merged.

You can install the extra requirements with dev option via pip install -e .[dev] assuming you navigated to the directory where setup.py is located.

@jnnr this PR should be merged soon to prevent merge conflicts due to the code linting.
I can add the same workflow to oemoflex after your review.

@jnnr
Copy link
Collaborator

jnnr commented Feb 18, 2021

@jnnr and @juliusmeier you can lint your code with Black (run black . --exclude docs/). The CI will remind you to do so, as the build will fail if the code is not linted with black, as soon as #25 is merged.

How do you do it? Do you have black added to your pre-commit hook? update: Did so and am happy with it.

@jnnr
Copy link
Collaborator

jnnr commented Feb 18, 2021

Shall we use flake8 as well? I am for it. To prevent conflicts with black, some rules should be ignored. I found information [here] (https://ljvmiranda921.github.io/notebook/2018/06/21/precommits-using-black-and-flake8/) and in this recent oemof-solph PR oemof/oemof-solph#746.

@SabineHaas
Copy link
Collaborator Author

@jnnr and @juliusmeier you can lint your code with Black (run black . --exclude docs/). The CI will remind you to do so, as the build will fail if the code is not linted with black, as soon as #25 is merged.

How do you do it? Do you have black added to your pre-commit hook? update: Did so and am happy with it.

nice 👍 could you post here, how you did that?

If you've followed instructions from your above posted link I understand that this can be added to the repo, so that it is a setting for all of us..?!

Shall we use flake8 as well? I am for it. To prevent conflicts with black, some rules should be ignored. I found information [here] (https://ljvmiranda921.github.io/notebook/2018/06/21/precommits-using-black-and-flake8/) and in this recent oemof-solph PR oemof/oemof-solph#746.

Ah yes, I can add this.

@jnnr
Copy link
Collaborator

jnnr commented Feb 18, 2021

How do you do it? Do you have black added to your pre-commit hook? update: Did so and am happy with it.

nice +1 could you post here, how you did that?

I added a file named pre-commit (no extension) to my local git repo's .git/hooks. The file contains update: Added the --diff flag to prevent black from actually converting the files. Before commit, I just want a check. Otherwise, black changes the files, but these changes are not part of the commit. Maybe there is a more clever way to automatically add it and then commit?

#!/bin/sh
flake8 .
black . --diff --exclude /docs

Maybe the order matters?

Then, I made the file executable by chmod +x

Now, each time when I commit, the linters run first and stop the commit if there is mistake.

@SabineHaas
Copy link
Collaborator Author

How do you do it? Do you have black added to your pre-commit hook? update: Did so and am happy with it.

nice +1 could you post here, how you did that?

I added a file named pre-commit (no extension) to my local git repo's .git/hooks. The file contains:

#!/bin/sh
flake8 .
black . --exclude /docs

Maybe the order matters?

Then, I made the file executable by chmod +x

Now, each time when I commit, the linters run first and stop the commit if there is mistake.

Nice thank you! I guess you could even push that to the repo, so that we can use it, as well.

Now you can use black ., no need for --exclude, as I've added a config file for black, too.

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.

[Ci] Travis not set up
2 participants