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

Error logging limited #112

Merged
merged 10 commits into from
Oct 16, 2018
Merged

Error logging limited #112

merged 10 commits into from
Oct 16, 2018

Conversation

evansiroky
Copy link

Round 1 of error logging refactor. Not as many changes as #108.

@codecov-io
Copy link

codecov-io commented Aug 12, 2018

Codecov Report

❗ No coverage uploaded for pull request base (dev@4b510bf). Click here to learn what that means.
The diff coverage is 11.57%.

Impacted file tree graph

@@          Coverage Diff          @@
##             dev    #112   +/-   ##
=====================================
  Coverage       ?   5.13%           
  Complexity     ?      91           
=====================================
  Files          ?     129           
  Lines          ?    6751           
  Branches       ?     880           
=====================================
  Hits           ?     347           
  Misses         ?    6379           
  Partials       ?      25
Impacted Files Coverage Δ Complexity Δ
...nveyal/datatools/common/status/MonitorableJob.java 0% <ø> (ø) 0 <0> (?)
...ols/editor/controllers/api/SnapshotController.java 0% <0%> (ø) 0 <0> (?)
...main/java/com/conveyal/gtfs/GraphQLController.java 0% <0%> (ø) 0 <0> (?)
...ols/manager/controllers/api/ProjectController.java 10.45% <0%> (ø) 2 <0> (?)
...atools/manager/controllers/api/UserController.java 7.77% <0%> (ø) 2 <0> (?)
.../datatools/manager/controllers/DumpController.java 0% <0%> (ø) 0 <0> (?)
...atools/manager/controllers/api/NoteController.java 5.55% <0%> (ø) 2 <0> (?)
.../manager/controllers/api/DeploymentController.java 0% <0%> (ø) 0 <0> (?)
...anager/controllers/api/OrganizationController.java 15.38% <0%> (ø) 2 <0> (?)
...ools/manager/controllers/api/StatusController.java 13.04% <0%> (ø) 2 <0> (?)
... and 10 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 4b510bf...f4f49bf. Read the comment docs.

@landonreed landonreed self-requested a review October 10, 2018 20:33
@landonreed landonreed self-assigned this Oct 10, 2018
Copy link
Member

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of changes.

* supplied details about the exception encountered.
*/
public static String formatJSON(String message, int code, Exception e) {
public static JsonNode getObjectNode(String message, int code, Exception e) {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps the return type should be ObjectNode here.

@@ -207,12 +207,13 @@ public static boolean updateSnapshotMetadata (String jsonString) {

/**
* Load a v2 JSON dump (i.e., objects with the class structure immediately before the MongoDB migration).
* @param req
Copy link
Member

Choose a reason for hiding this comment

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

Remove param from javadoc if it has no description.

*/
private static boolean loadLegacy(String jsonString) {
private static boolean loadLegacy(Request req) {
Copy link
Member

Choose a reason for hiding this comment

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

There may be cases in the future where it's helpful to be able to just load a JSON string directly into this loadLegacy method (rather than going through a spark request). Could you keep the bulk of this function's logic and the previous function signature intact (single String param) and maybe just wrap this within the standard (Request req, Response res) function signature for the spark request?

@landonreed landonreed assigned evansiroky and unassigned landonreed Oct 12, 2018
@evansiroky
Copy link
Author

The latest PR comments should be addressed and this should be ready to be merged.

@evansiroky evansiroky assigned landonreed and unassigned evansiroky Oct 13, 2018
@landonreed landonreed merged commit 1db34f2 into dev Oct 16, 2018
@landonreed landonreed deleted the error-logging-limited branch October 16, 2018 20:47
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