Skip to content

Feature/#165 file and line number in report#172

Merged
redcatbear merged 11 commits into
developfrom
feature/#165_File_and_line_number_in_report
Oct 13, 2018
Merged

Feature/#165 file and line number in report#172
redcatbear merged 11 commits into
developfrom
feature/#165_File_and_line_number_in_report

Conversation

@redcatbear

Copy link
Copy Markdown
Collaborator

@kaklakariada please review

{
this.reportService.reportTraceToPath(trace, output, format, this.verbosity, this.newline);
this.reportService.reportTraceToPath(trace, output, format, this.verbosity, this.newline,
this.showOrigin);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This argument list gets longer and longer. Maybe introduce a parameter object?

@codecov

codecov Bot commented Sep 29, 2018

Copy link
Copy Markdown

Codecov Report

Merging #172 into develop will increase coverage by 0.12%.
The diff coverage is 94.23%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop     #172      +/-   ##
=============================================
+ Coverage      94.31%   94.44%   +0.12%     
- Complexity      1004     1045      +41     
=============================================
  Files            106      108       +2     
  Lines           2745     2879     +134     
  Branches         231      243      +12     
=============================================
+ Hits            2589     2719     +130     
- Misses            98      103       +5     
+ Partials          58       57       -1
Impacted Files Coverage Δ Complexity Δ
...sallcode/openfasttrace/report/html/HtmlReport.java 95.45% <ø> (ø) 5 <0> (ø) ⬇️
...llcode/openfasttrace/exporter/ExporterFactory.java 80.95% <ø> (ø) 12 <0> (ø) ⬇️
...llcode/openfasttrace/importer/ImporterContext.java 100% <100%> (ø) 4 <1> (ø) ⬇️
...org/itsallcode/openfasttrace/cli/CliArguments.java 93.1% <100%> (+0.65%) 33 <3> (+3) ⬆️
...ode/openfasttrace/cli/commands/ConvertCommand.java 100% <100%> (ø) 4 <3> (ø) ⬇️
...ttrace/report/view/html/HtmlSpecificationItem.java 94.52% <100%> (+2.85%) 35 <4> (+8) ⬆️
...llcode/openfasttrace/exporter/ExporterService.java 100% <100%> (ø) 3 <1> (ø) ⬇️
...rc/main/java/org/itsallcode/openfasttrace/Oft.java 100% <100%> (ø) 1 <1> (?)
...llcode/openfasttrace/importer/ImporterService.java 100% <100%> (ø) 4 <1> (-1) ⬇️
...de/openfasttrace/cli/commands/AbstractCommand.java 76.66% <100%> (+5.83%) 8 <1> (+1) ⬆️
... and 18 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 6eed764...d30c90b. Read the comment docs.

@redcatbear

Copy link
Copy Markdown
Collaborator Author

@kaklakariada: implemented your change. Reworked the API in the course. Updated samples in the user giude.

Please review again.

@kaklakariada kaklakariada left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Merge conflicts fixed.


protected List<SpecificationItem> importItems()
{
this.oft.addInputs(this.toPaths(this.arguments.getInputs()));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be possible to add the inputs to the ImportSettings class?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done.


public class OftRunner implements Oft
{
private final List<Path> inputs = new ArrayList<>();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Moving inputs to the ImportSettings would make this class stateless

import org.junit.Test;

/**
* This class implements a parameter object to control the settings of OFT's

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment does not fit to class (only mentioned once)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Removed from test.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Deleted.

@redcatbear

Copy link
Copy Markdown
Collaborator Author

@kaklakariada, please check again.

*/
public Builder addInputs(final Path... inputs)
{
for (final Path input : inputs)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You could do: this.addInputs(asList(inputs));

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That would add another Import and creates an intermediate ArrayList so I consider this solution better here.

@redcatbear
redcatbear merged commit 2bc48e7 into develop Oct 13, 2018
@redcatbear
redcatbear deleted the feature/#165_File_and_line_number_in_report branch October 13, 2018 14:16
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