Remove deprecated constructor and use exposed builder pattern by MiSikora · Pull Request #483 · ChuckerTeam/chucker

MiSikora · 2020-10-23T19:39:49Z

📄 Context

There is an issue - #462.

📝 Changes

This implementation uses more robust builder pattern in terms of future development. It should be used with 4.x. There is one inconvenience when it comes to default values but it is not that big of a deal. I'll describe it more in the comments to the code.

🚫 Breaking

Yes it breaks both API and ABI, but it is for 4.x.

🛠️ How to test

~~I suggest reviewing commits separately. With 5e7d733 you can check if ReplaceWith works correctly.~~

Nothing special to test.

⏱️ Next steps

Closes #462

Helps with #466. It does not solve it, but I don't think we can fix it unfortunately. We can only make sure that in the future we don't fall into the same trap. Though suggestion with adding ABI compatibility tool would be a nice addition.

We should consider doing the same with ChuckerCollector and RetentionManger. They do not change that dynamically, so there might be no reason to switch there to the builder pattern but on the other hand it might pose at some point in the future same issues as #466.

MiSikora · 2020-10-23T19:42:02Z

This is the inconvenience that I mentioned. Default fields are declared here instead of in the builder. It is only due to test purposes. Instantiating ChuckerCollector would require a lot of mocking of Context that I don't want to be bothered with.

On the other hand it is nice to not create stuff eagerly in the builder.

vbuberen · 2020-10-25T21:43:24Z

I like that we will have builder patter in Chucker soon.
However, I have a few concerns/suggestions about this PR.

I don't like how we are removing existing constructor without proper deprecation process. It is not necessary that we need ReplaceWith point to something specific, but at least deprecation messages, like we did for throwables would be good.
With introduced changes and added deprecation messages (if we add some) I believe that we should change the version to 3.4.0, not 3.3.1 to follow semantic versioning approach.
While looking through code I thought that ChuckerInterceptor looks quite filled with different stuff now. What if we just create some interface for the public API, which will be the single entry point for user? In such case we could create some separate Builder class to implement this interface and to build and return an actual configured ChuckerInterceptor object. Might be an overkill and lead us to more breaking changes, but we might end up with better separation of concerns.

MiSikora · 2020-10-25T21:51:56Z

I don't like how we are removing existing constructor without proper deprecation process. It is not necessary that we need ReplaceWith point to something specific, but at least deprecation messages, like we did for throwables would be good.

With introduced changes and added deprecation messages (if we add some) I believe that we should change the version to 3.4.0, not 3.3.1 to follow semantic versioning approach.

I'm not sure if I understand what you mean by proper deprecation process. If it is connected to your second point I get it. Otherwise can you explain how should it look like? Constructor is marked as @Deprecated and has a message for 3.x.

While looking through code I thought that ChuckerInterceptor looks quite filled with different stuff now. What if we just create some interface for the public API, which will be the single entry point for user? In such case we could create some separate Builder class to implement this interface and to build and return an actual configured ChuckerInterceptor object. Might be an overkill and lead us to more breaking changes, but we might end up with better separation of concerns.

Can you create some snippet? I think I don't get the full picture.

vbuberen · 2020-10-26T08:19:59Z

I'm not sure if I understand what you mean by proper deprecation process.

I mean that we are just removing some existing part of our public API without any warnings. Yes, I agree that we don't make people do a ton of changes, but still, it would be better if we had some announcement/warning and some grace period when we have both deprecated constructor and the brand new builder. It feels kind of tricky since we have just one ChuckerInterceptor class which is being edited in this PR, but still.

Can you create some snippet? I think I don't get the full picture.

Yes, I should do some, probably. Also need to connect all dots to prove that the concept is valid.

MiSikora · 2020-10-26T08:33:53Z

I mean that we are just removing some existing part of our public API without any warnings. Yes, I agree that we don't make people do a ton of changes, but still, it would be better if we had some announcement/warning and some grace period when we have both deprecated constructor and the brand new builder.

Maybe it got lost in the whole PR description. After merging this PR without squashing I want to cherry-pick 5e7d733 onto 3.x where old constructor is still present and just deprecated. Users will have a grace period for as long as 4.x is not released.

vbuberen · 2020-10-26T09:55:31Z

Yes, looks like I missed the point about deprecations in one of commits.

In such case I am fine with the approach. But still, we would need to use 3.4.0 instead of 3.3.1 as we are introducing some new deprecations and some new API.

MiSikora · 2020-10-26T09:57:55Z

Yes, it's a little bit unfortunate that we'd have to make next release with a minor bump but I want to remove the burden of @JvmOverloads constructor from 4.x.

vbuberen

Reviewed this PR by commits (as it supposed to happen) and everything looks fine and reasonable for me.

As to my suggestion about additional interfaces - we can come back to it later when we do other changes to transaction processing.

cortinico · 2020-11-03T00:07:59Z

Great job 👍 Also sorry for being late to the game here.

Though suggestion with adding ABI compatibility tool would be a nice addition.

+1 on this. I actually wanted to take a stab to add https://github.com/Kotlin/binary-compatibility-validator to Chucker in the near future (that will prevent future problems like this one).

2. With introduced changes and added deprecation messages (if we add some) I believe that we should change the version to 3.4.0, not 3.3.1 to follow semantic versioning approach.

Agree here on what @vbuberen mentioned. If we merge this, we need to release 3.4.0 as a deprecation (of our prominent API) is worth a minor bump rather than a patch-fix bump.

Imho this PR should also be split in 2 separate PRs: 5e7d733 should be pointed against master while 6d4a8b4 should be pointed against 4.x (or however we wish to call it). As a rule of thumb is also useful for our users to have separate PRs that are easier to browse.

MiSikora · 2020-11-03T06:14:29Z

Imho this PR should also be split in 2 separate PRs: 5e7d733 should be pointed against master while 6d4a8b4 should be pointed against 4.x (or however we wish to call it). As a rule of thumb is also useful for our users to have separate PRs that are easier to browse.

I thought that we treat now our develop as a 4.x. That's why I wanted to merge these two commit without squashing them, and then create a separate PR for 3.x with some of commits that should also land in 3.x cherry-picked from develop.

cortinico · 2020-11-03T07:48:24Z

I thought that we treat now our develop as a 4.x.

(I wrote master but I meant develop sorry)

Yup I'm fine either way. But I would still advice we follow this flow:

Create a PR with 5e7d733 & Merge it on develop
Release version to 3.4.0 (<- this will be the point where we start the 3.x branch).
Create a new PR with 6d4a8b4 & Merge it on develop

MiSikora · 2020-11-03T08:04:51Z

Ok, got it. I was just a little bit confused because we already have 3.x but I we didn't anticipate next minor bump so it's all fine then. I'll split this into two separate PRs.

cortinico · 2020-11-03T08:20:59Z

Ok, got it. I was just a little bit confused because we already have 3.x

Sorry, I also haven't noticed that. In that case, let's create a PR with 5e7d733 & Merge it on 3.x

MiSikora · 2020-11-03T08:34:15Z

Ok, I added @VisibleForTesting and created #491. 6d4a8b4 changed to a923a8f.

MiSikora · 2020-11-04T18:12:49Z

Ok, I changed PR target. I think after merging #491 it will automatically retarget to develop branch. If not I'll update it then.

MiSikora · 2020-11-05T13:46:32Z

Some conflicts popped up. I'll resolve them after 3.4.0 is released.

cortinico · 2020-11-05T23:40:31Z

Let's rebase this on top of #496 once it's merged

cortinico

LGTM 👍

MiSikora added the enhancement New feature or improvement to the library label Oct 23, 2020

MiSikora added this to the 3.3.1 milestone Oct 23, 2020

MiSikora commented Oct 23, 2020

View reviewed changes

MiSikora force-pushed the builder-pattern branch from 6a24281 to 3168614 Compare October 23, 2020 19:44

MiSikora requested review from cortinico and vbuberen October 23, 2020 19:45

MiSikora force-pushed the builder-pattern branch from 3168614 to 9674246 Compare October 23, 2020 19:52

MiSikora changed the title ~~Builder pattern~~ Use builder pattern for ChuckerInterceptor Oct 23, 2020

MiSikora force-pushed the builder-pattern branch from 9674246 to 6d4a8b4 Compare October 24, 2020 07:59

vbuberen approved these changes Oct 26, 2020

View reviewed changes

MiSikora mentioned this pull request Nov 3, 2020

Use builder pattern for ChuckerInterceptor in a backwards compatible way #491

Merged

MiSikora force-pushed the builder-pattern branch from 6d4a8b4 to a923a8f Compare November 3, 2020 08:32

MiSikora removed this from the 3.4.0 milestone Nov 4, 2020

MiSikora changed the base branch from develop to builder-pattern-3.x November 4, 2020 18:09

MiSikora changed the title ~~Use builder pattern for ChuckerInterceptor~~ Remove deprecated constructor and use expose builder pattern only Nov 4, 2020

MiSikora added the ⛔️ do not merge A PR that should not be merged label Nov 4, 2020

MiSikora changed the title ~~Remove deprecated constructor and use expose builder pattern only~~ Remove deprecated constructor and use exposed builder pattern Nov 4, 2020

Base automatically changed from builder-pattern-3.x to develop November 5, 2020 13:45

MiSikora force-pushed the builder-pattern branch from a923a8f to 67167aa Compare November 5, 2020 16:03

MiSikora removed the ⛔️ do not merge A PR that should not be merged label Nov 5, 2020

MiSikora force-pushed the builder-pattern branch from 67167aa to 5070c64 Compare November 5, 2020 16:04

MiSikora added this to the 4.0.0 milestone Nov 6, 2020

Remove ChuckerInterceptor constructor deprecated in 3.4.0

ec8db89

MiSikora force-pushed the builder-pattern branch from 5070c64 to ec8db89 Compare November 6, 2020 17:51

cortinico approved these changes Nov 9, 2020

View reviewed changes

MiSikora merged commit ffe771f into develop Nov 9, 2020

MiSikora deleted the builder-pattern branch November 9, 2020 13:27

Uh oh!

Conversation

MiSikora commented Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📄 Context

📝 Changes

🚫 Breaking

🛠️ How to test

⏱️ Next steps

Uh oh!

MiSikora Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbuberen commented Oct 25, 2020

Uh oh!

MiSikora commented Oct 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vbuberen commented Oct 26, 2020

Uh oh!

MiSikora commented Oct 26, 2020

Uh oh!

vbuberen commented Oct 26, 2020

Uh oh!

MiSikora commented Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vbuberen left a comment

Choose a reason for hiding this comment

Uh oh!

cortinico commented Nov 3, 2020

Uh oh!

MiSikora commented Nov 3, 2020

Uh oh!

cortinico commented Nov 3, 2020

Uh oh!

MiSikora commented Nov 3, 2020

Uh oh!

cortinico commented Nov 3, 2020

Uh oh!

MiSikora commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MiSikora commented Nov 4, 2020

Uh oh!

MiSikora commented Nov 5, 2020

Uh oh!

cortinico commented Nov 5, 2020

Uh oh!

cortinico left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MiSikora commented Oct 23, 2020 •

edited

Loading

MiSikora Oct 23, 2020 •

edited

Loading

MiSikora commented Oct 25, 2020 •

edited

Loading

MiSikora commented Oct 26, 2020 •

edited

Loading

MiSikora commented Nov 3, 2020 •

edited

Loading