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

Repository Reorganization #124

Merged
merged 26 commits into from
Feb 8, 2022
Merged

Repository Reorganization #124

merged 26 commits into from
Feb 8, 2022

Conversation

trevorb1
Copy link
Member

@trevorb1 trevorb1 commented Feb 7, 2022

In this pull request, I have:

  • Reorganized the folder structure to follow snakemake guidelines
  • Added configuration files to deploy all scripts in workflow/scripts as a package in preparation of adding in pytest functionality
  • Linted all files in workflow/ to follow pylint standards. Snakemake files pass snakemake --lint checks
  • Broke out snakefile into separate rule files (*.smk) to follow snakemake's best practice guidelines
  • Added in github action to check pylint standards for a push on any branch and a pr on the main branch (checks python, yaml, and snakemake files)
  • Changed folder location to execute workflow. In the root folder (ie. Canada-U.S.-ElecTrade/), the command snakemake data_file -c1 now executes the workflow

No functional changes have been made to the model. The CanadaUSA.txt data file that the workflow produces has not changed with the updates

Copy link
Collaborator

@Kiel-SFU Kiel-SFU left a comment

Choose a reason for hiding this comment

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

This is a very comprehensive pull request, good job!

As we talked about, it probably should've been broken into at least two PR's, and the README's should really be updated alongside (to limit historical "broken" versions of the code on GitHub), but it looks good overall.

@@ -0,0 +1,26 @@
[metadata]
name = can_usa_scripts
version = 0.0.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are currently not autoversioning, but it can seemingly be achieved with Git: https://stackoverflow.com/questions/37814286/how-to-manage-the-version-number-in-git. Do you think we should use versioning, either by updating this value or through a different method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel free to add as an issue. I don't think it is necessary for this pr tho?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, it's not necessary for this PR. Issue #128 created for looking into this.

@trevorb1 trevorb1 requested a review from Kiel-SFU February 8, 2022 15:39
Copy link
Collaborator

@Kiel-SFU Kiel-SFU 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 created a series of issues that came up in this code review, but I've tested these current changes and we're good to merge!

@Kiel-SFU Kiel-SFU merged commit a2313ff into DeltaE:main Feb 8, 2022
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.

2 participants