Skip to content

Conversation

@rjrudin
Copy link
Contributor

@rjrudin rjrudin commented Nov 17, 2022

Was able to remove this completely in OkHttpServices, as the argument wasn't used for anything. But can't remove it from ResourceServices since that's a public interface. So deprecating the argument in the 6 methods that have it.

Adjusted each test and example not to pass in a mimetype argument anymore; those were misleading since it implied that the argument was having some impact.

@rjrudin
Copy link
Contributor Author

rjrudin commented Nov 17, 2022

@ehennum Curious if you have any thoughts on the approach of deprecating this in ResourceServices - I don't think we can remove them because that interface is public, unless we consider it a bug that the argument exist (I am open to that interpretation too - I look at it as a bug because it's suggesting to the user that passing in a value will have an impact, but it won't).

Copy link
Contributor

@BillFarber BillFarber left a comment

Choose a reason for hiding this comment

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

I think you must have enjoyed removing all that code.

@rjrudin
Copy link
Contributor Author

rjrudin commented Nov 18, 2022

Going to keep this open until @ehennum can weigh in on the deprecate vs remove-because-bugfix approach

@rjrudin rjrudin force-pushed the feature/1036-mimetypes branch from 02d93df to b1533f6 Compare November 18, 2022 15:38
Copy link
Contributor

@ehennum ehennum left a comment

Choose a reason for hiding this comment

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

Good to finalize this cleanup!

@rjrudin
Copy link
Contributor Author

rjrudin commented Nov 22, 2022

@ehennum Thoughts though on deprecate vs remove? I think there's an argument for remove without first deprecating it:

  1. This would be done in a new major release
  2. The existence of the args can be viewed as a bug from a user perspective - i.e. any user who has tried to pass something in has most likely noticed that it has no impact, and thus does not work, and thus is a bug

@ehennum
Copy link
Contributor

ehennum commented Nov 22, 2022

@rjrudin, I see belatedly that the current documentation doesn't deprecate the mimetypes parameter:

https://docs.marklogic.com/javadoc/client/index.html?overview-summary.html

In general, I believe the preferred approach is to document the deprecation and remove on the next release cycle to give adopters time to change their code.

In the specific case, given that the parameter is a noop, removing it wouldn't remove any functionality. So, the concern would be limited to forcing people to change their code and recompile if they are using the noop.

Given that the change is so straightforward, the argument could be made that there's no good reason for adopters to defer the fix during a deprecation period.

Was able to remove this completely in OkHttpServices, as the argument wasn't used for anything. But can't remove it from ResourceServices since that's a public interface. So deprecating the argument in the 6 methods that have it. 

Adjusted each test and example not to pass in a mimetype argument anymore; those were misleading since it implied that the argument was having some impact.
@rjrudin rjrudin force-pushed the feature/1036-mimetypes branch from b1533f6 to d9aa09c Compare November 22, 2022 17:36
@rjrudin rjrudin merged commit 74d1fb7 into develop Nov 22, 2022
@rjrudin rjrudin deleted the feature/1036-mimetypes branch November 22, 2022 17:36
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.

4 participants