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

FindBugs fix; Windows build fix. #271

Merged
merged 6 commits into from
Jan 7, 2017

Conversation

cameronxm
Copy link
Contributor

@cameronxm cameronxm commented Oct 19, 2016

FindBugs flagged access to a static field from a non-static context; introduced reference updater class and singleton instance. Also added a small change to make the tests work on Windows. (Since Windows is not a sufficiently enterprise platform, you may regard this part of the PR as optional.)

@codecov-io
Copy link

codecov-io commented Oct 19, 2016

Current coverage is 89.56% (diff: 83.33%)

Merging #271 into master will increase coverage by 4.36%

@@             master       #271   diff @@
==========================================
  Files            61         61          
  Lines           358        345    -13   
  Methods           0          0          
  Messages          0          0          
  Branches         21         21          
==========================================
+ Hits            305        309     +4   
+ Misses           46         29    -17   
  Partials          7          7          

Powered by Codecov. Last update f98ec95...c386ab8

@emiln
Copy link
Member

emiln commented Oct 20, 2016

Hi, @cameronxm, and thank you for your interest in FizzBuzzEnterpriseEdition. We are very excited about the proposed changes, but a few things need addressing before we can merge:

  1. The indentation in the changes seems to be a mixture of tabs and spaces, which has sent several senior developers into a frothing rage. Line 28 in the file src/main/java/com/seriouscompany/business/java/fizzbuzz/packagenamingpackage/impl/ApplicationContextHolder.java was found particularly egregious, and our senior Spring developer, Tim, flat out refuses to merge the changes before this is fixed.
  2. Business has for reasons that elude regular developers taken to a new obsession about code coverage and has descended upon all decreases with a blind zeal that leaves absolutely no wiggle room.I would suggesting adding superfluous, covered lines until the changeset in this PR becomes a net gain in total code coverage. It seems unlikely that business will be reasonable about the matter.

I hope we can quickly fix these trifles and see your great changes make it into the project.

@emiln emiln merged commit 4590711 into EnterpriseQualityCoding:master Jan 7, 2017
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.

5 participants