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

Moved CommandContext and CommandResult to command package #193

Merged
merged 7 commits into from
Feb 28, 2022

Conversation

msarvar
Copy link

@msarvar msarvar commented Feb 24, 2022

Created a new command package to contain command to contain everything command specific.

Moved CommandContext, CommandResult, CommandLock, CommandName, ProjectCommandContext, ProjectResult, and CommandResult into the package.
Removed Command from all structs.
For instance:

# before
models.CommandContext{}
# after
command.Context{}

Both ProjectCommandContext and ProjectResults have a dependency on CommandName so I had to move them together to avoid cyclical dependencies

Copy link
Contributor

@nishkrishnan nishkrishnan left a comment

Choose a reason for hiding this comment

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

Why are we moving this to models? I'm not a fan of that models package because it inevitably becomes a dumping ground. Let's keep data models in meaningful packages. Like in this case, maybe keeping it in a package called command and then the objects could just be Context and Result? Wdyt?

@msarvar
Copy link
Author

msarvar commented Feb 24, 2022

I like that, I was moving to models just to get it out of events. I like moving them to command package. Should I keep it in events or create workflows and put it there?

Copy link
Contributor

@nishkrishnan nishkrishnan left a comment

Choose a reason for hiding this comment

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

Sweet looks good.

@msarvar msarvar merged commit d10ec3f into release-v0.17.3-lyft.1 Feb 28, 2022
@msarvar msarvar deleted the move-structs-to-models branch February 28, 2022 22:28
@msarvar msarvar changed the title Moved CommandContext and CommandResult to models Moved CommandContext and CommandResult to command package Mar 1, 2022
Aayyush pushed a commit that referenced this pull request Mar 22, 2022
* Moved CommandContext and CommandResult to models (#193)

* Moved CommandContext and CommandResult to models

* move from models to command

rename CommandContext -> Context
rename CommandResult -> Result

* moved command related helpers into command package

* move ProjectCommandContext and ProjectResult to command/project package

* move project command context and project result

* revert unrelated code

* move tests

* fix left over

* fix linting

* fix tests

* remove unused import

* fix project context dependencies

* fix depenedecies

* fix typo
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