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

todos should be falsy #821

Merged
merged 2 commits into from
Apr 6, 2018
Merged

todos should be falsy #821

merged 2 commits into from
Apr 6, 2018

Conversation

owocki
Copy link
Contributor

@owocki owocki commented Apr 6, 2018

@mbeacom i noticed this when dockerizing my setup: i think that the TODOs in the settings file shoudl be falsy, so that settings.GITHUB_CLIENT_ID (and others) are falsy... and we don't try to send out a bunch of github/twitter/slack notifications because of lines of code like this everywhere.

@owocki owocki requested a review from mbeacom April 6, 2018 17:40
@codecov
Copy link

codecov bot commented Apr 6, 2018

Codecov Report

Merging #821 into master will increase coverage by 1.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #821      +/-   ##
==========================================
+ Coverage   34.34%   35.46%   +1.12%     
==========================================
  Files          99      102       +3     
  Lines        5768     6524     +756     
  Branches      674      878     +204     
==========================================
+ Hits         1981     2314     +333     
- Misses       3702     4111     +409     
- Partials       85       99      +14
Impacted Files Coverage Δ
app/app/settings.py 79.8% <100%> (-1.7%) ⬇️
app/faucet/views.py 22.34% <0%> (-3.31%) ⬇️
app/app/urls.py 91.66% <0%> (-1.89%) ⬇️
app/external_bounties/views.py 17.24% <0%> (-0.86%) ⬇️
app/app/context.py 0% <0%> (ø) ⬆️
app/github/urls.py 100% <0%> (ø)
app/github/views.py 24.61% <0%> (ø)
app/github/middleware.py 0% <0%> (ø)
app/app/utils.py 20.37% <0%> (+0.21%) ⬆️
... and 5 more

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 0b3639a...ae65f3c. Read the comment docs.

Copy link
Contributor

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

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

I'm good with this! One thing, though... couldn't we use '' for string types and it still return false without the possibility of passing NoneType where str is expected (if we're not checking if object is ? The main reason I used TODO for the default values was to maintain parity from the original local_settings.py/settings.py defaults.

@owocki
Copy link
Contributor Author

owocki commented Apr 6, 2018

@mbeacom good call.. just made that change.

Copy link
Contributor

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

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

lgtm :shipit:

@mbeacom mbeacom merged commit acf1bb4 into master Apr 6, 2018
@mbeacom mbeacom deleted the kevin/todo branch April 6, 2018 18:39
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