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

Save backup refactor #324

Closed
wants to merge 9 commits into
base: trunk
from

Conversation

Projects
None yet
3 participants
@ham1

ham1 commented Nov 9, 2017

Further refactoring for readability to createBackupFile and other minor improvements.

Description

Refactoring createBackupFile and minor tidy up of surrounding code.
I've got a bit carried away but tried to commit often so that at least some of the changes can be easily reviewed and applied.

Motivation and Context

The aim is to improve readability and thus make fixing, testing and improving easier.

How Has This Been Tested?

Manually with both backup settings set. Unit testing is still not very easy.

Screenshots (if appropriate):

Types of changes

  • Code improvement

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.
@codecov-io

This comment has been minimized.

codecov-io commented Nov 9, 2017

Codecov Report

Merging #324 into trunk will increase coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk     #324      +/-   ##
============================================
+ Coverage     57.75%   57.75%   +<.01%     
- Complexity     9913     9915       +2     
============================================
  Files          1139     1139              
  Lines         73051    73050       -1     
  Branches       7303     7300       -3     
============================================
+ Hits          42187    42188       +1     
+ Misses        28392    28390       -2     
  Partials       2472     2472
Impacted Files Coverage Δ Complexity Δ
src/core/org/apache/jmeter/gui/action/Save.java 13.25% <0%> (+0.07%) 4 <0> (ø) ⬇️
...ocol/jms/org/apache/jmeter/protocol/jms/Utils.java 54.83% <0%> (-2.16%) 15% <0%> (ø)
...apache/jmeter/extractor/TestBoundaryExtractor.java 97.88% <0%> (ø) 17% <0%> (ø) ⬇️
...c/core/org/apache/jmeter/reporters/Summariser.java 86.15% <0%> (+0.76%) 19% <0%> (+1%) ⬆️
...s/org/apache/jmeter/timers/PoissonRandomTimer.java 78.37% <0%> (+5.4%) 10% <0%> (+1%) ⬆️

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 20e8392...487cc43. Read the comment docs.

@asfgit asfgit closed this in 5737230 Nov 15, 2017

@pmouawad

This comment has been minimized.

Contributor

pmouawad commented Nov 15, 2017

Thanks @ham1 ,
Could you review my commit as your PR had a conflict after merging of Workbench before it.

I think I didn't miss anything but another eye is welcome.
Regards

@ham1

This comment has been minimized.

ham1 commented Nov 16, 2017

@pmouawad Thank you for committing my changes - in future I'm happy to rebase and fix merge issues so you don't have to :) (just let me know via comment or email)

There were only a few minor things:

  • formatting of streams api code - they seem to be mostly on one line, whereas I think they read much better when split, (perhaps this is the eclipse formatter?)
  • some extra whitespace was added (maybe eclipse settings?)
  • I question the JavaDoc on the private methods - it didn't seem to help, I had hoped the method name was sufficient - is there a reason it was added?
  • formatting of JavaDoc comments, I think we should agree on a format and make sure it's updated in the style guide on the wiki as we currently have a mix and it would be good to agree to prevent further inconsistency (the location of the description of the param, same or new line)

What's the best way for me to provide these changes? Force push to this PR, new PR, email patch file?

Thanks

@ham1 ham1 deleted the ham1:save_backup_refactor branch Nov 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment