Skip to content

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

Merged
kjac merged 3 commits intorelease/14.0from
v14/bugfix/datatypeservice-getallasync
May 10, 2024
Merged

v14: Return all data types from DataTypeService.GetAllAsync()#16230
kjac merged 3 commits intorelease/14.0from
v14/bugfix/datatypeservice-getallasync

Conversation

@ronaldbarendse
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@kjac kjac deleted the v14/bugfix/datatypeservice-getallasync branch May 10, 2024 06:12
@ronaldbarendse
Copy link
Copy Markdown
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