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

add-scripts-folder #133

Merged
merged 21 commits into from
Jul 20, 2021
Merged

Conversation

aminalaee
Copy link
Member

@aminalaee aminalaee commented Jul 16, 2021

I think it'd be clear to move all the scripts to the scripts folder following this.
I've moved all existing scripts and updated the github workflow. But here some questions/ideas:

  • Should we move piccolo.sh from root of project too?
  • Coverage is not enforced, should we add a minimum coverage so pipeline fails on missing coverage using something like Codecov?
  • release.sh can be a workflow in github to upload to PyPi when github release is created.

@dantownsend
Copy link
Member

dantownsend commented Jul 16, 2021

@aminalaee This is much cleaner, thanks!

I've moved piccolo.sh into the scripts folder.

I used to have Coveralls configured when I was using Travis CI. I couldn't easily get it working on GitHub Actions, and didn't have time to investigate further. If we could get it working, that would be great.

Likewise, with releasing to PyPI - I didn't have time to investigate how to store secrets with GitHub Actions. I moved over in a hurry when Travis CI became unbearably slow!

@aminalaee
Copy link
Member Author

@dantownsend Glad to be of help.
I'll give it a try tomorrow and let yoy know how it goes.

@codecov-commenter
Copy link

codecov-commenter commented Jul 17, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@af8d2d4). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #133   +/-   ##
=========================================
  Coverage          ?   89.85%           
=========================================
  Files             ?      106           
  Lines             ?     4901           
  Branches          ?        0           
=========================================
  Hits              ?     4404           
  Misses            ?      497           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af8d2d4...e18344f. Read the comment docs.

@aminalaee
Copy link
Member Author

aminalaee commented Jul 17, 2021

@dantownsend
Sorry this got a bit messy.

  • I removed coverage-report.sh and added coverage report and coverage validation in test-postgres.sh and test-sqlite.sh. Looks like different coverages exist for two databases.
  • I've added the release.yaml, you need to get PyPI API token and set up PYPI_TOKEN secret for the project or organization in the Settings -> Secret.
  • If you don't mind I added codecov, instead of Coveralls The results are not uploading to codecov yet. You need to set CODECOV_TOKEN secret for the project or organization
  • Added isort and black to lint check to keep the codebase consistent.
  • Added .flake8 and pyprojcet.toml configs to configure black and flake8. Removed # noqa: F401 from all __init__ files and set flake8 config to ignore that in __init__ files. But I guess a more pythonic way would be to add __all__
    to __init__s so variables are not unused anymore.

@lgtm-com
Copy link

lgtm-com bot commented Jul 17, 2021

This pull request introduces 1 alert and fixes 1 when merging 7f466a1 into bd54ed7 - view on LGTM.com

new alerts:

  • 1 for 'import *' may pollute namespace

fixed alerts:

  • 1 for 'import *' may pollute namespace

@lgtm-com
Copy link

lgtm-com bot commented Jul 17, 2021

This pull request introduces 1 alert and fixes 1 when merging 689cc4e into bd54ed7 - view on LGTM.com

new alerts:

  • 1 for 'import *' may pollute namespace

fixed alerts:

  • 1 for 'import *' may pollute namespace

@lgtm-com
Copy link

lgtm-com bot commented Jul 17, 2021

This pull request introduces 1 alert and fixes 1 when merging 27f4287 into bd54ed7 - view on LGTM.com

new alerts:

  • 1 for 'import *' may pollute namespace

fixed alerts:

  • 1 for 'import *' may pollute namespace

@lgtm-com
Copy link

lgtm-com bot commented Jul 17, 2021

This pull request fixes 3 alerts when merging ef88f57 into bd54ed7 - view on LGTM.com

fixed alerts:

  • 3 for 'import *' may pollute namespace

@dantownsend
Copy link
Member

@aminalaee Thanks for all your hard work with this - it's a big improvement.

With isort and black, I heard they're sometimes incompatible. Have you ever experienced this?

@aminalaee
Copy link
Member Author

aminalaee commented Jul 17, 2021

@dantownsend I really enjoy working with piccolo. Glad to work on it.
After you mentioned that I did some lookup and found this and this.
Apparently isort version 5 introduced a profile compatibe with black. I'll try that.

@aminalaee
Copy link
Member Author

aminalaee commented Jul 18, 2021

@dantownsend Added isort profile for black. The profile makes them pretty compatible. The only difference is line length, which isort and flake8 use 79 by default, but black uses 88. Since piccolo was already using 79, I set 79 explicitly for isort, black and flake8.

I created issues for other repos to implement the same structure when we can agree on something.

@lgtm-com
Copy link

lgtm-com bot commented Jul 18, 2021

This pull request fixes 3 alerts when merging c29464d into bd54ed7 - view on LGTM.com

fixed alerts:

  • 3 for 'import *' may pollute namespace

@lgtm-com
Copy link

lgtm-com bot commented Jul 18, 2021

This pull request fixes 3 alerts when merging e18344f into bd54ed7 - view on LGTM.com

fixed alerts:

  • 3 for 'import *' may pollute namespace

@dantownsend
Copy link
Member

@aminalaee Thanks again for all this. I've added secrets for Codecov and PyPI, so hopefully they should work as expected.

@dantownsend dantownsend merged commit 24fe814 into piccolo-orm:master Jul 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.

3 participants