Skip to content

Basic Ci/CD and fix code style #79

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

Merged
merged 10 commits into from
Nov 25, 2022
Merged

Basic Ci/CD and fix code style #79

merged 10 commits into from
Nov 25, 2022

Conversation

juanitorduz
Copy link
Collaborator

@juanitorduz juanitorduz commented Nov 21, 2022

Partially closes #57

This PR:

  • Add Makefile
  • Add lint and tests requirements
  • Add dummy test folder / file
  • Fix style (black + flake8 + isort) of the whole code base.

@drbenvincent, please double-check the small changes I had to do to fix the style. Feel free to push changes to the branch. I tried to change the least as possible.

Note that the tests and lint are passing on my fork: https://github.com/juanitorduz/CausalPy/actions/runs/3517816662/jobs/5896022154

@juanitorduz
Copy link
Collaborator Author

The lint check detected a duplicated name

/causalpy/data/simulate_data.py:101:1: F811 redefinition of unused 'generate_time_series_data' from line 70

@drbenvincent
Copy link
Collaborator

These are the only instances of generate_time_series_data in the repo and one of them is in the docstring, so a little confusing.
Screenshot 2022-11-23 at 20 38 44

@juanitorduz
Copy link
Collaborator Author

Ok! Thanks for checking! It might be I messed up something when resolving conflicts. I'll take a look 🙈

@juanitorduz
Copy link
Collaborator Author

juanitorduz commented Nov 23, 2022

But in line 66 you also have a generate_time_seried_data function right? (Same name ?)

@drbenvincent
Copy link
Collaborator

I'm not sure what's going on. From a search I only got 2 hits. But yes, I see now. Maybe just too much coding for one day!

@drbenvincent
Copy link
Collaborator

drbenvincent commented Nov 23, 2022

Do we need to add the generated .coverage to .gitignore, or does that need to be included in the repo?

Copy link
Collaborator

@drbenvincent drbenvincent left a comment

Choose a reason for hiding this comment

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

I've checked all this over and run the pre-commit checks locally, and the various make commands. Made one commit a moment ago to fix a duplicated function definition. Everything seems fine at this point.

I think you must have merged main into this branch after my updates on Wed evening, but if you can just confirm then I can approve and we can merge.

@juanitorduz
Copy link
Collaborator Author

@drbenvincent , in 5a74cd8 I added some changes which make the version setting easier. Also fixed some missing lint problems. If you see the ci/cd output now is green: https://github.com/juanitorduz/CausalPy/actions/runs/3539004142/jobs/5940424788

Please take a look into it and test locally by running make init.

@drbenvincent drbenvincent merged commit 988ac05 into pymc-labs:main Nov 25, 2022
@juanitorduz juanitorduz deleted the basic_ci branch November 25, 2022 13:07
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.

Run a tighter ship: Additional pre-commit checks and CI
2 participants