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

Add option to customize the hashed password field #31

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

fibrasek
Copy link

First of all, awesome project!

When working on a project I found the necessity to give other name to the hashed password field, so I thought of writing this PR.

Copy link
Owner

@BlakeWilliams BlakeWilliams left a comment

Choose a reason for hiding this comment

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

Thanks for opening this!

Overall it looks good, but I had one question about how to organize the new option in terms of functions.

"""
def hash_password(changeset) do
def hash_password(changeset, opts \\ %{}) do
Copy link
Owner

Choose a reason for hiding this comment

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

What are your thoughts on splitting this into two functions? The first could have the same name/arity as the original method and the second could match on your field name?

e.g.

def hash_password(changeset) do
  hash_password(changeset, field_name: :hashed_password)
end

def hash_password(changeset, %{"field_name" => field_name})
  # existing logic in PR
end

That would also let us focus the documentation for each logical case.

Copy link
Author

Choose a reason for hiding this comment

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

That looks even better and would not introduce breaking changes related to the arity, nice!

I'll make these changes :)

@fibrasek
Copy link
Author

@BlakeWilliams sorry about taking some much time to make those changes, but they're done :)

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