Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HistoryRequestParams fix #323

Merged
merged 3 commits into from May 1, 2019

Conversation

@withakay
Copy link
Collaborator

commented Apr 28, 2019

When upgrading to 1.1 HistoryRequestParams was replaced with PaginatedRequestParams (as part of a process of reducing code duplication) which has created a breaking change for people moving from 0.8.

The fix presented here is to create new HistoryRequestParams class that inherits from PaginatedRequestParams.

@withakay withakay requested a review from paddybyers Apr 28, 2019

@paddybyers
Copy link
Member

left a comment

LGTM, thanks

@mattheworiordan

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

0.8 -> 1.1 is a breaking release though. Are we happy to continue to support this type in spite of it not being anywhere in the spec? I appreciate it’s nice of us to provide forwards support, but equally major releases do give us an opportunity to improve the API and clean things up. I wasn’t involved in the discussion to add this type, but are we sure this is what we want?

@ndastur

This comment has been minimized.

Copy link

commented Apr 28, 2019

Just been pointed to this by Paddy. Agree clean up a good idea but 1.1 documents still refer to old class.

@withakay withakay changed the title Historyparams fix HistoryRequestParams fix Apr 28, 2019

@withakay

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 28, 2019

@mattheworiordan

Are we happy to continue to support this type in spite of it not being anywhere in the spec?

My feeling is the default position should be 'no'. We should, I think, assess individual cases and decide if it is worth the effort (or is even feasible). In this particular case the fix was trivial and has no obvious side effects (other than having us all working on a Sunday!). When it is that easy to be backwards compatible I think it is worth doing.

@mattheworiordan

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

When it is that easy to be backwards compatible I think it is worth doing.

Sure, that is perhaps worth it, but if we do that, shouldn't we:

  • Show a deprecation notice
  • Update the docs to use the new style

Either way, we need to think about whether it makes sense to support this if we didn't have existing users of an old API. If the answer is no, then we should deprecate and perhaps support in 1.1, 1.2 etc and drop in 2.0.

@withakay

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 29, 2019

@mattheworiordan I have added an [Obsolete] attribute to the HistoryRequestParams class, this will give a compile time warning (Warning CS0618: 'HistoryRequestParams' is obsolete: 'HistoryRequestParams may be removed in future versions, please use PaginatedRequestParams instead.') and in Visual Studio there will be a squiggle and popup.

image

(Yup, no idea why VS needs to display the warning twice 🤦‍♂️)

@mattheworiordan

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Looks good.

Are there any other param types that need updating too, such as StatsRequestParams?

@withakay

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 30, 2019

StatsRequestParams still exists and inherits from PaginatedReqestParams (Previously it inherited from HistoryRequestParams).

public class StatsRequestParams : PaginatedRequestParams

@mattheworiordan

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

StatsRequestParams still exists and inherits from PaginatedReqestParams

Ah, OK, and I see it extends PaginatedReqestParams so fine to leave as is.

@withakay withakay merged commit c0f40b8 into code-cleanup May 1, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@withakay withakay deleted the historyparams-fix branch May 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.