search

GeriLife / companionship

Promoting companionship and well-being for everyone.
European Union Public License 1.2
7 stars 18 forks source link

ci: add github workflow #61

Closed TheMythologist closed 2 years ago

TheMythologist commented 2 years ago

Related to #59

Unsure if you want to use black or autopep8 (or both), this workflow runs black.

brylie commented 2 years ago

@TheMythologist, thank you for this contribution :-)

I've added a small request about not auto-committing changes. I'm still a bit nervous about GitHub Actions being able to commit code to a repo without human review.

brylie commented 2 years ago

Unsure if you want to use black or autopep8 (or both), this workflow runs black.

What would be the best strategy, Black alone or Black + autopep8? What would be some added benefits with Black + autopep8?

TheMythologist commented 2 years ago

A good explanation would be this: https://blog.frank-mich.com/python-code-formatters-comparison-black-autopep8-and-yapf/

Note: The site is slightly outdated, black is now configurable with many options.

TLDR: Black formats the code more than pep-8 standards, whereas autopep8 only formats the code to follow pep-8 code styles. So formatting with autopep8 is redundant if black is already used to format code.

TheMythologist commented 2 years ago

Also apologies for the last 2 commits, I'm referencing black's documentation

brylie commented 2 years ago

Ok, this PR is looking good. Is it ready for review, or are you planning other changes?

Relatedly, what would you recommend using to measure and report on test coverage for this project?

TheMythologist commented 2 years ago

Yup the PR is ready :)

I would recommend coverage - it's can be easily integrated with most python testing tools such as pytest (which I personally use). Here's their documentation: https://coverage.readthedocs.io/en/6.4.4/

brylie commented 2 years ago

I would recommend coverage

Yes, that is also what Code Climate recommends. Do you think we could include adding Coverage in this pull request, as part of a comprehensive CI process? If so, I can configure the test reporter ID as an environment variable.

https://docs.codeclimate.com/docs/configuring-test-coverage

TheMythologist commented 2 years ago

Sorry but I'm not familiar enough with coverage to include it in this PR 😅

brylie commented 2 years ago

Sorry but I'm not familiar enough with coverage to include it in this PR

Me either. All good :-)

Is this PR ready for release now?

TheMythologist commented 2 years ago

Yup it is! You can enable Github workflows for PRs if u want to see the workflow in action. (you can also configure such that workflow requires approval to run for first-time contributors to the repository)

brylie commented 2 years ago

Oh no! I tried to resolve the conflicts, and it created a commit that made this branch out of sync with main. I've been rebasing into main, so it is necessary to rebase into other development branches, such as this, to keep things in sync. Could you rebase main into your local branch and force-push it to GitHub?