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

Move out integration processes #597

Merged
merged 10 commits into from
Dec 4, 2023
Merged

Conversation

sjspielman
Copy link
Member

Closes #592

A little 🍬 to get the ball rolling! This PR does what it says.

@sjspielman sjspielman requested a review from jashapiro November 28, 2023 14:18
Copy link
Member

@jashapiro jashapiro left a comment

Choose a reason for hiding this comment

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

I think there a few more things we need to do here before merging, so we still have a working workflow... Mostly we need to either remove or rename some things so that we are not calling processes that the script does not know about. (The alternative would be to import the module, but I don't think that is the direction we want to go at this point).

integrate.nf Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to rename this while you are at it, since it isn't integration at all now?

integrate.nf Outdated
@@ -180,4 +129,3 @@ workflow {
// generate integration report
integration_report(integrate_harmony.out, file(integration_template))
Copy link
Member

Choose a reason for hiding this comment

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

Probably want to rename this report, and we need to get rid of the integration process calls above here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe just comment this out for now, since we also don't have the rmd yet?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm fine with just commenting out most of this. I just don't like the idea of merging in changes that by their nature break the workflow.

Copy link
Member Author

Choose a reason for hiding this comment

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

and we need to get rid of the integration process calls above here?

Ah, I totally missed this 🙃. I blame my long 🦃 break. These lines are gone now!

@sjspielman
Copy link
Member Author

I think there a few more things we need to do here before merging

👏

@sjspielman
Copy link
Member Author

In actual response to your feedback here, I was wary about going too far here and making integrate -> merge changes, but I can certainly get that ball rolling as part of this PR too! But since you're on board with tackling now, I'll do some renaming and as you say, make sure the workflow still works/flows.

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently github felt this update was sufficiently different to call it a whole new file instead of a diff!

@sjspielman
Copy link
Member Author

Let me know what you think of this now! I made some changes to get merge language in there all around, but left the code mostly as-is to not break the workflow altogether. Do you suggest that I take it farther and/or back off a little bit?

@sjspielman sjspielman requested a review from jashapiro November 28, 2023 16:07
Copy link
Member

@jashapiro jashapiro left a comment

Choose a reason for hiding this comment

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

LGTM, but might as well add in the missing stub chunk!

merge.nf Outdated Show resolved Hide resolved
merge.nf Outdated Show resolved Hide resolved
@sjspielman sjspielman merged commit e2ac4a0 into development Dec 4, 2023
3 checks passed
@sjspielman sjspielman deleted the sjspielman/592-move-processes branch December 4, 2023 13:53
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