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

Github workflow for testing and linting. #54

Merged
merged 16 commits into from Dec 20, 2021
Merged

Conversation

wsad1
Copy link
Contributor

@wsad1 wsad1 commented Nov 26, 2021

Added workflows for linting and testing.

  • The testing workflow seems to pass. Do check out testing.yml and let me know if there's a better way to set things up.
  • The liniting workflow fails. Are there some flake8 options i need to use to prevent this?

@Actis92
Copy link
Contributor

Actis92 commented Nov 29, 2021

Hi @wsad1 talking with @manujosephv I have some suggestions:

  1. It's not useful to have 2 pipelines, one for the tests and one for the linting. It's better to use tox (like it was done in travis, and when one develop locally) If you want can make something similar:
    `- name: Install dependencies
    run: |
    python -m pip install --upgrade pip
    pip install tox
    • name: Test with tox
      run: tox`
  2. For the problems find with flake8 depends by @manujosephv how he want to manages. Using tox, inside the tox.ini is possible to add a section in order to ignore some rules. Obviously depends which rules he wants to ignore and which one he prefer to fix the code. For example:
    [flake8] ignore = E501

@manujosephv
Copy link
Owner

E501 for sure, and I see there are a few corrections to be done for linting.. Will have to take that up after merging @Actis92 PR. But lets only fail the overall workflow (i.e. the unit test and deployment when some serious errors like syntax errors E9,F63,F7,F82. And also ignore the files in examples folder from linting.

And we should also only build and upload to PyPi for commits with tags. Rest of the commits just have linting and unit test..

@Actis92
Copy link
Contributor

Actis92 commented Dec 1, 2021

For what concern build and upload on PyPI when a tags is pushed, in my repo I have used a different pipeline like this one in case can be helpful for you
`
name: "tagged-release"
on:
push:
tags:
- "v*"
jobs:
tagged-release:
name: "Tagged Release"
runs-on: "ubuntu-latest"

steps:
  - uses: "actions/checkout@master"
  - name: "Set up Python 3.9"
    uses: "actions/setup-python@v1"
    with:
      python-version: 3.9
  - uses: "marvinpinto/action-automatic-releases@latest"
    with:
      repo_token: "${{ secrets.GITHUB_TOKEN }}"
      prerelease: false
  - name: Install pypa/build
    run: >-
      python -m
      pip install
      build
      --user
  - name: Build a binary wheel and a source tarball
    run: >-
      python -m
      build
      --sdist
      --wheel
      --outdir dist/
  - name: Publish distribution 📦 to PyPI
    if: startsWith(github.ref, 'refs/tags')
    uses: pypa/gh-action-pypi-publish@master
    with:
      password: ${{ secrets.PYPI_API_TOKEN }}`

@manujosephv
Copy link
Owner

@wsad1 Were you able to review @Actis92 's comments?

@wsad1
Copy link
Contributor Author

wsad1 commented Dec 9, 2021

@Actis92 thanks for your comments.

  1. The reason I have separate files is so that we could have separate badges for linting and testing. But, if that's not needed I think we can combine the two files.
  2. Could you elaborate on the need for tox. As far as I know github actions can achieve most things tox can like testing on different environment, setting environment variables etc. Further we could use the setup.cfg to configure linting and testing if needed.

@manujosephv
Copy link
Owner

manujosephv commented Dec 9, 2021

@wsad1 Publishing the package to pypi is something that is still missing, am I right?

And as far as tox goes, if all the functionality is replicated by github actions, then I think it's okay. @Actis92, what do you think?

@wsad1
Copy link
Contributor Author

wsad1 commented Dec 9, 2021

@manujosephv yes deploying to pypi is still missing. I was planning to add that as a separate PR once we merge this.
But it looks like @Actis92 has a good solution. @Actis92 are you planning to create a PR for the deployment workflow or would you prefer I do it?

@wsad1 wsad1 marked this pull request as draft December 9, 2021 13:46
@Actis92
Copy link
Contributor

Actis92 commented Dec 9, 2021

@wsad1 for what concern the PR for the deployment workflow You can use my code, in this moment I'm not working on a PR. Instead for what concern tox and github actions, I know that can be configured to do almost the same job. My observation was in order to guarantee the consistency between local development and github actions, because the risk is that locally I execute tox before push the code but it can be configured differently from github actions. In any case for me it's ok to proceed also with the solution proposed by @wsad1 :)

@wsad1
Copy link
Contributor Author

wsad1 commented Dec 10, 2021

Thanks @Actis92 , this is a good point. The configurations in actions might be different.
We can make sure the flake8 and test configs are same for users and in github actions by using setup.cfg. Is my understanding correct?
Also if we do go the setup.cfg route we can probably remove tox.ini and also update the contributing guideline. @manujosephv what do you think.

@wsad1 wsad1 marked this pull request as ready for review December 16, 2021 07:27
Copy link
Owner

@manujosephv manujosephv left a comment

Choose a reason for hiding this comment

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

Looks good.. Approving..

@manujosephv
Copy link
Owner

Merging this PR. In the next PR let's include PyPi push, tox cleanup, and updating documentation.

@manujosephv manujosephv merged commit cb29e73 into manujosephv:main Dec 20, 2021
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.

None yet

3 participants