Opened 13 months ago

Closed 7 weeks ago

Last modified 7 weeks ago

#24419 closed enhancement (fixed)

Improve getter names for boolean fields

Reported by: karsten Owned by: metrics-team
Priority: Medium Milestone:
Component: Metrics/Onionoo Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: irl Sponsor:

Description

From #21827:

The method name getRecommendedVersion is quite misleading - especially together with getVersion - as it returns a boolean not a version value, but as the method was introduced long ago this shouldn't halt a merge.

Maybe, the naming could be improved at some point?

So, what should we rename here? All boolean/Boolean fields in docs package classes to isXY()?

Anything else?

Child Tickets

Change History (12)

comment:1 Changed 2 months ago by efgyirfe784

Please see https://gitlab.com/743zpnpGUq27GgR/onionoo/tree/24419-isXY for a proposed solution to this ticket.

Please note:

  1. I did not change src/test/org/torproject/metrics/onionoo/updater/DummyStatusEntry's "getUnmeasured()" to "isUnmeasured()" because the interface org.torproject.descriptor.NetworkStatusEntry requires it, and I figured org.torproject.descriptor is outside the scope of this ticket. Note however that many docs classes had a "getMeasured()" method that is now "isMeasured()", which differs slightly from NetworkStatusEntry's "getUnmeasured()".
  1. Several classes in org.torproject.metrics.onionoo.docs (like SummaryDocument) have a field called "isRelay" which has a getter method named "isRelay()". I think this makes sense because of what the field represents, but some folks prefer not to have field names repeated as method names. I didn't change this, just an FYI.

comment:2 Changed 2 months ago by karsten

Reviewer: karsten
Status: newneeds_review

Reviewing now.

comment:3 in reply to:  1 Changed 2 months ago by karsten

Reviewer: karsten

Replying to efgyirfe784:

Please see https://gitlab.com/743zpnpGUq27GgR/onionoo/tree/24419-isXY for a proposed solution to this ticket.

Looks good and is almost ready to be merged. The only changes I'd make are:

  • Append new change log entries to existing ones.
  • Start the new change log entry with a capital letter and end it with a period.
  • Squash the second commit into the first, because it only removes a file that was accidentally added in the first commit.

Please note:

  1. I did not change src/test/org/torproject/metrics/onionoo/updater/DummyStatusEntry's "getUnmeasured()" to "isUnmeasured()" because the interface org.torproject.descriptor.NetworkStatusEntry requires it, and I figured org.torproject.descriptor is outside the scope of this ticket.

Agreed. We'd first have to change metrics-lib interfaces, put out a new major metrics-lib release, and then update the tests in Onionoo. Worth doing, though we would ideally combine this with other changes that require a major metrics-lib version bump. Let's create a ticket before closing this one.

Note however that many docs classes had a "getMeasured()" method that is now "isMeasured()", which differs slightly from NetworkStatusEntry's "getUnmeasured()".

That's two different thing. One says whether a relay was measured, the other says what the measurement result was.

  1. Several classes in org.torproject.metrics.onionoo.docs (like SummaryDocument) have a field called "isRelay" which has a getter method named "isRelay()". I think this makes sense because of what the field represents, but some folks prefer not to have field names repeated as method names. I didn't change this, just an FYI.

We could maybe rename the field to relay. Or leave it as it is.

I'll again leave this here for irl to review, too, before merging it to master with the minor changes listed above. Thanks!

comment:4 Changed 2 months ago by irl

Reviewer: irl

comment:5 Changed 8 weeks ago by efgyirfe784

Done! (I think). I might've misunderstood "Append new change log entries to existing ones. " -> I moved the topmost changelog entry to just below the latest, but maybe this isn't what you were looking for?

I think leaving the field name as "isRelay" makes sense.

comment:6 Changed 8 weeks ago by efgyirfe784

I created child ticket #28114 to update metrics-lib.

comment:7 Changed 7 weeks ago by irl

Status: needs_reviewneeds_information

I'm not exactly sure what I'm reviewing here.

Can you give me a URL to clone and a branch name?

comment:8 Changed 7 weeks ago by efgyirfe784

Sorry, you should be able to clone this: https://gitlab.com/743zpnpGUq27GgR/onionoo.git and look at branch '24419-isXY'.

comment:9 Changed 7 weeks ago by irl

Status: needs_informationneeds_review

Ok thanks.

comment:10 Changed 7 weeks ago by irl

Status: needs_reviewmerge_ready

Looks ready for merge. There are two commits but really the second commit should just be a fixup. karsten will fix this when merging.

3530b53f3bb9c49bb089eb90869ff98fa7debb8c ok
0d9d22caf28d9c772e65113fd6a2af182498622c fixup

Thanks!

comment:11 Changed 7 weeks ago by karsten

Resolution: fixed
Status: merge_readyclosed

Thanks for the additional review. Squashed both commits and made some trivial changes to the change log. Edits to the change log and squashing commits are things I'm happy to make as part of merging. Thanks again for the patch! Closing.

comment:12 Changed 7 weeks ago by efgyirfe784

My pleasure, glad I could help!

Note: See TracTickets for help on using tickets.