-
Notifications
You must be signed in to change notification settings - Fork 13
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
Rename controllers context #54
Conversation
2a41e87
to
3335c3c
Compare
b521e87
to
0b4c8e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
%Result{ | ||
execution_id: execution_id, | ||
group_id: group_id | ||
} = result | ||
) do | ||
Wanda.Repo.insert!(%ExecutionResult{ | ||
Repo.insert!(%ExecutionResult{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't have Credo warned us about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Credo warns only if it is nested 2 or 3 levels.
Sometimes it is useful to alias with one level of nesting, for example when you have overlapping module names from different "contexts". This is not the case, tho.
""" | ||
@spec save_result(Result.t()) :: ExecutionResult.t() | ||
def save_result( | ||
@spec create_execution_result(Result.t()) :: ExecutionResult.t() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the strange this is that we sillt use ExecutionResult
@@ -13,7 +13,7 @@ | |||
execution_id = "00000000-0000-0000-0000-000000000001" | |||
group_id = "00000000-0000-0000-0000-000000000002" | |||
|
|||
Wanda.Results.save_result(%Wanda.Execution.Result{ | |||
Wanda.Results.create_execution_result(%Wanda.Execution.Result{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should start considering at some point to remove this seed
Rename controller actions and context to be as close as possible to Phoenix standards, so we can use the
resource
directive in the router.