Skip to content

add: constructor injection - #186

Merged
faif merged 5 commits into
faif:masterfrom
fkromer:constructor_injection
Apr 10, 2017
Merged

faif merged 5 commits into
faif:masterfrom
fkromer:constructor_injection

Conversation

@fkromer

@fkromer fkromer commented Apr 4, 2017

Copy link
Copy Markdown

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

codecov-io commented Apr 4, 2017 •

Copy link
Copy Markdown

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

fkromer commented Apr 6, 2017

Copy link
Copy Markdown
Author

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
Copy Markdown
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
Copy Markdown
Author

Choose a reason for hiding this comment

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

DONE

@faif faif Apr 10, 2017 •

Copy link
Copy Markdown
Owner

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

fkromer commented Apr 24, 2017

Copy link
Copy Markdown
Author

@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