Skip to content

Refactor: split IExecution out from IModule and rename vars to reflect new structure #117

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.

Sign up for GitHub

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

Jump to bottom
Merged
fangting-alchemy merged 4 commits into from
Jul 26, 2024
Merged

Refactor: split IExecution out from IModule and rename vars to reflect new structure #117

fangting-alchemy merged 4 commits into from
Jul 26, 2024

Conversation

@fangting-alchemy
Copy link
Collaborator

@fangting-alchemy fangting-alchemy commented Jul 26, 2024

Refactor interfaces to enable simpler implementation by getting rid of unnecessary function requirement.
Also rename vars to match the refactor.

@fangting-alchemy fangting-alchemy requested a review from a team July 26, 2024 19:13
@fangting-alchemy fangting-alchemy changed the title Refactor: interface refactor Refactor: split IExecution out from IModule Jul 26, 2024
jaypaik
jaypaik approved these changes Jul 26, 2024
View reviewed changes
Copy link
Collaborator

@jaypaik jaypaik left a comment

Choose a reason for hiding this comment

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

Would love to do another pass at names (and also names for all other things) at some point! installModule bothers me a bit now since we changed ModuleManifest to ExecutionManifest.

@fangting-alchemy
Copy link
Collaborator Author

fangting-alchemy commented Jul 26, 2024

Would love to do another pass at names (and also names for all other things) at some point! installModule bothers me a bit now since we changed ModuleManifest to ExecutionManifest.

Yeah, was thinking about it. Maybe rename it to installExecution?

@jaypaik
Copy link
Collaborator

jaypaik commented Jul 26, 2024

installExecution

Perhaps. It does pair nicely with installValidation...

@adamegyed
Copy link
Contributor

adamegyed commented Jul 26, 2024

+1 to installExecution from me!

@adamegyed
Copy link
Contributor

adamegyed commented Jul 26, 2024

Also flagging that we should start targeting the develop branch instead of v0.8-develop, and we should delete v0.8-develop.

@jaypaik
Copy link
Collaborator

jaypaik commented Jul 26, 2024

Also flagging that we should start targeting the develop branch instead of v0.8-develop, and we should delete v0.8-develop.

Ah yes, great catch. @fangting-alchemy could you update the base branch on this PR?

@fangting-alchemy fangting-alchemy changed the base branch from v0.8-develop to develop July 26, 2024 20:09
@fangting-alchemy fangting-alchemy changed the title Refactor: split IExecution out from IModule Refactor: split IExecution out from IModule and rename vars to reflect new structure Jul 26, 2024
@fangting-alchemy fangting-alchemy merged commit 3f562f5 into develop Jul 26, 2024
@fangting-alchemy fangting-alchemy deleted the fangting/interfaces branch July 26, 2024 20:11
@adamegyed adamegyed mentioned this pull request Aug 1, 2024
Merged
adamegyed pushed a commit that referenced this pull request Aug 5, 2024
@fangting-alchemy @adamegyed
Refactor: split IExecution out from IModule and rename vars to reflec…
6571f29 
…t new structure (#117)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaypaik jaypaik jaypaik approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@fangting-alchemy @jaypaik @adamegyed