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
Imrpove code quality #6
Conversation
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 is a nice addition here, and I'm fully in favor of using it throughout Sugar. A couple inline questions re: Elixir versions and some now-unused code, but otherwise LGTM.
.travis.yml
Outdated
@@ -1,8 +1,9 @@ | |||
language: elixir | |||
elixir: | |||
- 1.2 |
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.
Do we really want to drop the test case for Elixir 1.2 here? We're still testing Sugar itself on that version.
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.
Good point. Adding 1.2
back in to the build matrix
:json -> [{"accept", "application/json"}] | ||
:xml -> [{"accept", "application/xml"}] | ||
# :json -> [{"accept", "application/json"}] | ||
# :xml -> [{"accept", "application/xml"}] |
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.
Are these supposed to be removed? If so, should we consider getting rid of the add_header
entirely, since it's unused at this point?
At the very least, we could prefix it as _add_header
in the function signature and just resort to header = []
for the time being.
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.
These weren't used really anyways, but I've commented them here (as opposed to removing) because I'm circling back to actually implement this in the near future.
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.
Gotcha. Carry on then. :)
LGTM. Merge it 👍 |
/cc @YellowApple