Opened 2 years ago

Closed 22 months ago

#22154 closed enhancement (implemented)

Remove code that was deprecated in the 1.x series

Reported by: karsten Owned by: iwakeh
Priority: Medium Milestone: metrics-lib 2.0.0
Component: Metrics/Library Version:
Severity: Normal Keywords:
Cc: iwakeh Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When we release version 2.0.0 we should remove all (?) code that was deprecated in the 1.x series.

Really all? What about code that we deprecated in the very last 1.x version just a few days before releasing 2.0.0? I believe even that, because whoever updates from 1.x to 2.x knows that things might break. And even if they update from an earlier 1.x, they'll need to watch out for changes. But I could be convinced otherwise.

Child Tickets

Change History (12)

comment:1 Changed 22 months ago by iwakeh

One focus point is DescriptorParserImpl and other places using DescriptorFile.

comment:2 Changed 22 months ago by iwakeh

Owner: changed from metrics-team to iwakeh
Status: newaccepted

comment:3 Changed 22 months ago by iwakeh

Cc: iwakeh added
Status: acceptedneeds_review

This task needs a four hands/four eyes approach; there are so many changes which seem to cause 'code-blindness' after a while.

A first branch here has three commits:

  • one removing all deprecation warnings.
  • a second that makes tests and benchmark compile
  • a third that makes all tests except one pass (testDirectorySignaturesLinesTwoAlgorithms in RelayNetworkStatusVoteImplTest fails)

Please review and change accordingly; maybe this needs another coding/review alteration.

comment:4 Changed 22 months ago by iwakeh

Just a second, I forgot to amend some tiny checkstyle issues.

Last edited 22 months ago by iwakeh (previous) (diff)

comment:5 Changed 22 months ago by iwakeh

Checkstyle doesn't complain anymore.
begin of task-22154 implementation

comment:6 Changed 22 months ago by karsten

I'm starting to review commits c57517c, 5148de2, and a56c96d now. Please base future amendments, if any, on those three commits.

comment:7 Changed 22 months ago by karsten

There, I pushed a first commit with tweaks to my task-22154 branch. This commit fixes issues that I found in your first commit and in the surrounding code. I have a few more thoughts on your second and third commit that I'll write another commit for later today. This is not exactly a fixup/squash commit, but if you don't mind leaving it as a separate commit, I don't mind either. Will work more on this ticket in 1--2 hours from now.

comment:8 in reply to:  7 Changed 22 months ago by iwakeh

Yes, leaving the single commits is easier and documents clearly which changes are intended and which not, i.e. it leads to better code in the end.

comment:9 Changed 22 months ago by karsten

There, I pushed a second commit with tweaks to the same branch. This time I focused on your second and third commit. Please take a look! If you agree with the changes, I'll push everything to master, as is.

comment:10 Changed 22 months ago by iwakeh

Status: needs_reviewmerge_ready

Looks, fine. All tests and checks pass, coverage doesn't complain.

I think the test structure should be adapted to use parameterized tests instead of the endless methods that actually only vary data. This is a new ticket though.

Merge ready!

comment:11 Changed 22 months ago by iwakeh

The mentioned new ticket is #22733.

comment:12 Changed 22 months ago by karsten

Resolution: implemented
Status: merge_readyclosed

Perfect! Pushing to master (Git server is really slow today). And I agree that our tests could be improved. So much to do! Thanks again! Closing.

Note: See TracTickets for help on using tickets.