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.

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

Validation of behavior and implementation modules #52

Merged
merged 1 commit into from
Oct 7, 2020
Merged

Validation of behavior and implementation modules #52

merged 1 commit into from
Oct 7, 2020

Conversation

saneery
Copy link
Contributor

@saneery saneery commented Oct 3, 2020 •
edited
Loading

👋
relates to #22

@saneery
Copy link
Contributor Author

saneery commented Oct 5, 2020

@msz Hi! How can I trigger CI?

msz
msz reviewed Oct 5, 2020
View reviewed changes
Copy link
Owner

@msz msz left a comment

Choose a reason for hiding this comment

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

Great work, thank you! Just a small comment.

lib/hammox.ex Outdated
Utils.replace_user_types(typespec, behaviour_module_name)
end)

:error ->
Copy link
Owner

Choose a reason for hiding this comment

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

What is the reason behind handling :error here? I don't see a use case for this function to silently proceed when the Code.Typespec.fetch_callbacks(behaviour_module_name) call errors — I'd rather make it fail to be stricter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, looks like the Code.Typespec.fetch_callbacks(behaviour_module_name) calls error only then the module doesn't exist. So I think there is a need to add validation of behavior module inside protect/2 function and raise an exception.

@msz
Copy link
Owner

msz commented Oct 5, 2020

Don't worry about CI, it doesn't run on PRs from forks for security

@msz
Copy link
Owner

msz commented Oct 7, 2020

I merged #51, could you rebase and resolve the conflict?

@saneery
Copy link
Contributor Author

saneery commented Oct 7, 2020

Done! :)

@saneery
Copy link
Contributor Author

saneery commented Oct 7, 2020 •
edited
Loading

@msz could you add hacktoberfest-accepted labels to my PRs? :)
here is instructions https://hacktoberfest.digitalocean.com/hacktoberfest-update

@msz msz merged commit ecb52c6 into msz:master Oct 7, 2020
@msz msz mentioned this pull request Oct 7, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msz msz msz approved these changes

Assignees
No one assigned
Labels
hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@saneery @msz