Skip to content

Clear up Name and Id parameters #3810

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

Merged
merged 5 commits into from
Jun 12, 2019
Merged

Clear up Name and Id parameters #3810

merged 5 commits into from
Jun 12, 2019

Conversation

codebrain
Copy link
Contributor

@codebrain codebrain commented Jun 11, 2019

We could consider consolidating further by using Id and Ids, but my concern here is that we would be allowing string types for ids that only accept long values, hence the specialisations.

@Mpdreamz Mpdreamz mentioned this pull request Jun 11, 2019
15 tasks
codebrain and others added 3 commits June 11, 2019 15:36
- Policy now uses Id
- Ids type was not defined, so removed, StringIds renamed to Ids
@Mpdreamz
Copy link
Member

@codebrain I've added some commits that allow us to expose params as string long, long? directly to the user for the cases that don't benefit from IUrlParameter without having them either rely on implicit conversion to a IUrlParameter implementation.

@Mpdreamz
Copy link
Member

Taking it to its logical conclusion and removing Name amongst others reminded me why we made this wrapped

  GetAliasUrlTests.cs(29, 43): [CS0121] The call is ambiguous between the following methods or properties: 'GetAliasRequest.GetAliasRequest(Names)' and 'GetAliasRequest.GetAliasRequest(Indices)'

In cases where an API has multiple route that take a single string you quickly turn into these kinds of compiler issues.

Reverted allowing string and long directly

@codebrain codebrain merged commit a9b2fb1 into 7.x Jun 12, 2019
@Mpdreamz Mpdreamz deleted the fix/consolidate-ids branch June 17, 2019 11:57
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.

2 participants