Skip to content

Add filename to graph report - #4984

Merged
leonardehrenfried merged 7 commits into
opentripplanner:dev-2.xfrom
HBTGmbH:add-filename-to-graph-report
Mar 30, 2023
Merged

Add filename to graph report#4984
leonardehrenfried merged 7 commits into
opentripplanner:dev-2.xfrom
HBTGmbH:add-filename-to-graph-report

Conversation

@bas-hbt

@bas-hbt bas-hbt commented Mar 20, 2023

Copy link
Copy Markdown
Contributor

Summary

adds the file path to the dataImportIssue-message-output of the graph report to localize errors more easily

@t2gran t2gran added this to the 2.3 milestone Mar 22, 2023
@bas-hbt
bas-hbt marked this pull request as ready for review March 24, 2023 08:28
@bas-hbt
bas-hbt requested a review from a team as a code owner March 24, 2023 08:28

@leonardehrenfried leonardehrenfried left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we want to add mutable state to the issue store. @t2gran and @vpaturet can advise how to implement this without it.

There are two possibilities that I can think of:

  1. pass the file name down the call stack
  2. make a custom implementation of the issue store that has a fixed file name, instantiate it per file and merge the results with the main one afterwards

This would be a good topic for Gitter, the dev meeting or a GH issue.

@leonardehrenfried

Copy link
Copy Markdown
Member

Please resolve the conflicts.

@t2gran t2gran added the !Improvement A functional improvement or micro feature label Mar 29, 2023

@t2gran t2gran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

After looking at this, I think the approach is ok. But, I have a few suggestions for improvment.

…e the source, rename filename to currentSource, rename methods to indicate the intention, add javadoc to the renamed methods
Comment thread src/main/java/org/opentripplanner/graph_builder/issue/api/IssueWithSource.java Outdated
Comment thread src/main/java/org/opentripplanner/graph_builder/issue/api/IssueWithSource.java Outdated
Comment thread src/main/java/org/opentripplanner/graph_builder/issue/api/IssueWithSource.java Outdated
Comment thread src/main/java/org/opentripplanner/graph_builder/issue/api/IssueWithSource.java Outdated
Comment thread src/main/java/org/opentripplanner/netex/NetexBundle.java
… that it delegates the type and message to the wrapped issue
@codecov

codecov Bot commented Mar 30, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 30.00% and project coverage change: +0.05 🎉

Comparison is base (5e54056) 64.08% compared to head (8b84a12) 64.13%.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #4984      +/-   ##
=============================================
+ Coverage      64.08%   64.13%   +0.05%     
- Complexity     13592    13617      +25     
=============================================
  Files           1675     1676       +1     
  Lines          66255    66418     +163     
  Branches        7149     7164      +15     
=============================================
+ Hits           42459    42598     +139     
- Misses         21422    21435      +13     
- Partials        2374     2385      +11     
Impacted Files Coverage Δ
.../graph_builder/issue/api/DataImportIssueStore.java 100.00% <ø> (ø)
...anner/graph_builder/issue/api/IssueWithSource.java 0.00% <0.00%> (ø)
.../org/opentripplanner/netex/mapping/QuayMapper.java 86.20% <0.00%> (-3.08%) ⬇️
...der/issue/service/DefaultDataImportIssueStore.java 66.66% <25.00%> (-33.34%) ⬇️
...ph_builder/issue/api/NoopDataImportIssueStore.java 85.71% <100.00%> (+5.71%) ⬆️
...in/java/org/opentripplanner/netex/NetexBundle.java 93.10% <100.00%> (+0.12%) ⬆️

... and 22 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bas-hbt
bas-hbt requested review from leonardehrenfried and t2gran and removed request for leonardehrenfried and t2gran March 30, 2023 12:10
@leonardehrenfried
leonardehrenfried merged commit 8947f06 into opentripplanner:dev-2.x Mar 30, 2023
t2gran pushed a commit that referenced this pull request Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Improvement A functional improvement or micro feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants