Skip to content

Conversation

@fkromer
Copy link

@fkromer fkromer commented Apr 4, 2017

quick and dirty implementation (needs some cleanup) in a new subdirectory for "design for testability" patterns. tested as single file testrun with Python 2.7 only (just let travis do the job...)

tested as single file testrun with Python 2.7 only
@codecov-io
Copy link

codecov-io commented Apr 4, 2017

Codecov Report

Merging #186 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #186      +/-   ##
==========================================
+ Coverage   94.51%   94.59%   +0.07%     
==========================================
  Files          55       57       +2     
  Lines        2333     2366      +33     
==========================================
+ Hits         2205     2238      +33     
  Misses        128      128
Impacted Files Coverage Δ
tests/test_constructor_injection.py 100% <100%> (ø)
dft/constructor_injection.py 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 520e9b1...4cca032. Read the comment docs.

@fkromer
Copy link
Author

fkromer commented Apr 6, 2017

ready for review


def get_current_time_as_html_fragment(self):
current_time = self.time_provider.now()
current_time_as_html_fragment = "<span class=\"tinyBoldText\">" + current_time + "</span>"
Copy link
Owner

Choose a reason for hiding this comment

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

It's better to use .format here instead of + (true for all string concatenations that you do)

Copy link
Author

Choose a reason for hiding this comment

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

DONE

Copy link
Owner

@faif faif Apr 10, 2017

Choose a reason for hiding this comment

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

Thanks but that's not what I meant. The idea is to avoid string concatenations completely since a) strings are immutable and there is some overhead included, b) it is more idiomatic in Python to use .format. The idea looks like that: "<span class=\"tinyBoldText\">{}{}</span>".format(current_time.hour, current_time.minute)

Copy link
Author

Choose a reason for hiding this comment

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

Isn't that exactly what I changed in commit 4cca032?

Copy link
Owner

Choose a reason for hiding this comment

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

Oops, sorry, I missed that :)

Copy link
Author

Choose a reason for hiding this comment

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

It is already late 😉

Copy link
Author

Choose a reason for hiding this comment

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

thanks for reviewing

@faif faif merged commit d3ea46e into faif:master Apr 10, 2017
@fkromer fkromer deleted the constructor_injection branch April 10, 2017 21:22
@fkromer
Copy link
Author

fkromer commented Apr 24, 2017

@faif Thanks for merging, but I miss constructor_injection.py here and test_constructor_injection.py here.

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.

3 participants