Skip to content

Conversation

@joshsmith
Copy link
Contributor

What's in this PR?

Changes if/else style to pattern matching.

Copy link

@sbatson5 sbatson5 left a comment

Choose a reason for hiding this comment

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

Just 1 question. Otherwise, LGTM 👍

end
def call(conn, _opts), do: conn |> identify

def identify(%{assigns: %{current_user: user}} = conn) do
Copy link

Choose a reason for hiding this comment

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

Can identify be private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sbatson5 I guess there's no reason why it can't be. Can you adjust your review to needs changes then so I know to change it?

Copy link

Choose a reason for hiding this comment

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

Lol. I don't know how to do that, haha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add another review that's a needs changes.

Copy link

@sbatson5 sbatson5 left a comment

Choose a reason for hiding this comment

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

Can we make the identify method private for both cases?

@joshsmith
Copy link
Contributor Author

@sbatson5 ready for another review.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 94.669% when pulling 78a2b01 on refactor-analytics-identify-plug into f8a1fae on develop.

@joshsmith joshsmith force-pushed the refactor-analytics-identify-plug branch from 78a2b01 to 9ad4078 Compare November 20, 2016 18:33
@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.084% when pulling 9ad4078 on refactor-analytics-identify-plug into 7ebaf31 on develop.

@joshsmith joshsmith force-pushed the refactor-analytics-identify-plug branch from 9ad4078 to 4e13dd0 Compare November 20, 2016 18:39
@joshsmith joshsmith dismissed sbatson5’s stale review November 20, 2016 18:39

Made changes requested

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.056% when pulling 4e13dd0 on refactor-analytics-identify-plug into 7b88660 on develop.

@joshsmith joshsmith merged commit c6cc27a into develop Nov 20, 2016
@joshsmith joshsmith deleted the refactor-analytics-identify-plug branch November 20, 2016 18:43
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.

4 participants