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

Users + Code coverage & CI (minus coverage files) #26

Merged
merged 1 commit into from
May 2, 2017

Conversation

jackcarlisle
Copy link
Member

@jackcarlisle jackcarlisle commented Apr 6, 2017

ready for review

@jackcarlisle jackcarlisle mentioned this pull request Apr 6, 2017
2 tasks
@codecov-io
Copy link

codecov-io commented Apr 6, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@5a68388). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##             master    #26   +/-   ##
=======================================
  Coverage          ?   100%           
=======================================
  Files             ?      5           
  Lines             ?     14           
  Branches          ?      0           
=======================================
  Hits              ?     14           
  Misses            ?      0           
  Partials          ?      0
Impacted Files Coverage Δ
web/router.ex 100% <ø> (ø)
web/models/user.ex 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a68388...3855d3d. Read the comment docs.

@@ -22,3 +22,6 @@ erl_crash.dump
# secrets file as long as you replace its contents by environment
# variables.
/config/prod.secret.exs

# Coverage
cover
Copy link
Member

Choose a reason for hiding this comment

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

thanks for ignoring the directory. 👍

@@ -37,7 +42,9 @@ defmodule Feedback.Mixfile do
{:phoenix_html, "~> 2.6"},
{:phoenix_live_reload, "~> 1.0", only: :dev},
{:gettext, "~> 0.11"},
{:cowboy, "~> 1.0"}]
{:cowboy, "~> 1.0"},
{:comeonin, "~> 2.0"},
Copy link
Member

@nelsonic nelsonic Apr 7, 2017

Choose a reason for hiding this comment

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

we only need comeonin if we are actually letting people save a password, which isn't the case yet, right? 🤔
From what I can tell, this PR does not include a form to actually allow a user to register...

Copy link
Member Author

Choose a reason for hiding this comment

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

@nelsonic we'll need it for a user to log in because we require the checkpw function it provides

Copy link
Member Author

@jackcarlisle jackcarlisle Apr 7, 2017

Choose a reason for hiding this comment

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

here's where we're using it in a subsequent PR https://github.com/dwyl/feedback/pull/33/files#diff-651f440688d3528ab409efd7e368c15fR37 (bit pre-emptive of me)

Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

@jackcarlisle looks good. totes merging. thanks! 🎉

@nelsonic nelsonic merged commit ddc0bfb into master May 2, 2017
@nelsonic nelsonic deleted the remove-coverage branch May 2, 2017 09:54
jackcarlisle added a commit that referenced this pull request May 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants