Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#19613 closed task (implemented)

Onionoo should confirm to style guide

Reported by: iwakeh Owned by: iwakeh
Priority: Medium Milestone: Onionoo 3.1-1.0.0
Component: Metrics/Onionoo Version:
Severity: Normal Keywords:
Cc: iwakeh Actual Points:
Parent ID: #19611 Points:
Reviewer: Sponsor:

Description

verify that Onionoo complies to the rules.
make necessary changes.

Child Tickets

Change History (22)

comment:1 Changed 3 years ago by karsten

Owner: set to karsten
Status: newassigned

Okay, I'll grab this one, too.

comment:2 Changed 3 years ago by karsten

Cc: iwakeh added
Status: assignedneeds_review

Wheee, please review my task-19613 branch.

comment:3 Changed 3 years ago by iwakeh

Status: needs_reviewneeds_revision

Wow, almost 1400 changes! Good to remove usage of deprecated items.

Changes can be merged.

What's open:

  • add cobertura task
  • change file structure

comment:4 Changed 3 years ago by iwakeh

Summary: confirm to style guideOnionoo should confirm to style guide

comment:5 in reply to:  3 Changed 3 years ago by karsten

Replying to iwakeh:

Wow, almost 1400 changes! Good to remove usage of deprecated items.

Changes can be merged.

Awesome, thanks for reviewing all the things! Merged! :)

What's open:

  • add cobertura task
  • change file structure

Would you want to do those?

comment:6 Changed 3 years ago by iwakeh

Owner: changed from karsten to iwakeh
Status: needs_revisionassigned

Taken.

comment:7 Changed 3 years ago by iwakeh

Milestone: Onionoo 3.1.1

comment:8 Changed 3 years ago by karsten

Owner: changed from iwakeh to karsten

I'll grab this one back and will start by writing missing Javadoc comments. Just in case you already made progress on this one, please shout!

comment:9 Changed 3 years ago by karsten

Status: assignedneeds_review

Please find my updated branch task-19613 with a commit that resolves all remaining Javadoc-related warnings. Also feel free to grab and resolve the remaining issues if you want.

comment:10 Changed 3 years ago by karsten

Update: pushed another commit to that same branch that fixes a bug I found while writing Javadocs.

comment:11 Changed 3 years ago by karsten

(Preparing another commit to resolve more checkstyle issues now..)

comment:12 Changed 3 years ago by karsten

There, pushed another commit to my branch that either resolves or suppresses the remaining warnings. Please review.

comment:13 Changed 3 years ago by iwakeh

Status: needs_reviewneeds_revision

Great, the javadocs will be really useful when working on this code next.
All checkstyle complaints gone.

When upgrading to descriptor 1.3.0 the tests won't compile (the move to descriptor 1.3.0 is not part of the guide doc, but maybe good to do it now):

src/test/java/org/torproject/onionoo/updater/DummyConsensus.java:16: error: DummyConsensus is not abstract and does not override abstract method getSignatures() in RelayNetworkStatusConsensus

Open change is the file structure according to the guide.

comment:14 Changed 3 years ago by iwakeh

And, could you add a test for the setFingerprint method, where the bugfix was?

comment:15 Changed 3 years ago by karsten

Status: needs_revisionassigned

Thanks for looking! Upgraded to metrics-lib 1.3.0 and added the test for setFingerprint() (see updated branch), but left the file structure change open. Would you want to do the latter again?

comment:16 Changed 3 years ago by iwakeh

Owner: changed from karsten to iwakeh

Sure.

comment:17 Changed 3 years ago by iwakeh

Status: assignedneeds_review

Please review the changes in my branch, which is based on yours above.

I adapted dependencies for those libs that are also used by descriptor. Jessy8 could be updated too, but that would need more testing.

Shall vagrant be removed and the how-to created for in the wiki instead?

comment:18 in reply to:  17 Changed 3 years ago by karsten

Status: needs_reviewassigned

Replying to iwakeh:

Please review the changes in my branch, which is based on yours above.

Looks good, merged!

I adapted dependencies for those libs that are also used by descriptor. Jessy8 could be updated too, but that would need more testing.

Looks good, too. Agreed that the Jetty update might need some testing first.

Shall vagrant be removed and the how-to created for in the wiki instead?

Yes, sounds good.

Leaving open until all issues are resolved or copied over to new tickets.

Thanks!

comment:19 Changed 3 years ago by iwakeh

Status: assignedneeds_review

Here's a vagrant free branch :-)
And, here the beginning of the contributor guide doc incl. vagrant information.

Please review.

(ticket for newer jetty: #19751)

comment:20 Changed 3 years ago by karsten

Status: needs_reviewassigned

Looks good, merged. Thanks!

comment:21 Changed 3 years ago by iwakeh

Resolution: implemented
Status: assignedclosed

All done.
Closing.
Thanks!

comment:22 Changed 3 years ago by karsten

Milestone: Onionoo 3.1.1Onionoo 3.1-1.0.0

Milestone renamed

Note: See TracTickets for help on using tickets.