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

v14: Return all data types from DataTypeService.GetAllAsync() #16230

Merged
merged 3 commits into from
May 10, 2024

Conversation

ronaldbarendse
Copy link
Contributor

PR #14665 added GetAllAsync(params Guid[] keys) to IDataTypeService, but the implementation returns 0 data types when no keys are specified, which is different to the existing GetAll(params int[] ids) that returns all data types of no IDs are specified.

This PR fixes the GetAllAsync() implementation and also removes some unnecessary async/awaits (on methods that use Task.FromResult().

@kjac kjac removed the request for review from nikolajlauridsen May 10, 2024 05:08
@kjac
Copy link
Contributor

kjac commented May 10, 2024

This is a dangerous change of behaviour, but I get the reasoning behind. Fortunately our tests uncovered the breakage 😄 I have amended the PR to ensure that we don't accidentally load all data types when we mean to load none.

Copy link
Contributor

@kjac kjac left a comment

Choose a reason for hiding this comment

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

💪 👍

@kjac kjac changed the base branch from release/14.0 to v14/dev May 10, 2024 06:11
@kjac kjac changed the base branch from v14/dev to release/14.0 May 10, 2024 06:11
@kjac kjac merged commit c7328bc into release/14.0 May 10, 2024
11 of 13 checks passed
@kjac kjac deleted the v14/bugfix/datatypeservice-getallasync branch May 10, 2024 06:12
@ronaldbarendse
Copy link
Contributor Author

This is a dangerous change of behaviour, but I get the reasoning behind. Fortunately our tests uncovered the breakage 😄 I have amended the PR to ensure that we don't accidentally load all data types when we mean to load none.

Hmm, very true indeed! We could fix this by adding an overload without the parameter, so GetAllAync() returns all, but GetAllAsync(Array.Empty<Guid>()) returns nothing, as it explicitly uses the key parameter (that would allow for removing your amends again)...

To avoid any confusion, a better solution would probably be to have separate method names, e.g. GetAllAsync() and GetAllByKeysAsync(params Guid[] keys) (or even just GetAsync(params Guid[] keys), although that would change the return type based on how many/which parameters you specify).

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