EES-5593 Public API wildcard data set versioning #5577
Conversation
…dataSetVersion parameters endpoints support.
|
|
||
| namespace GovUk.Education.ExploreEducationStatistics.Common.Utils; | ||
|
|
||
| public record DataSetVersionRecord(int? Major, int? Minor, int? Patch) |
There was a problem hiding this comment.
DataSetVersionNumber would probably be a better name for this?
Suffixing names with 'record' is also a bit funky and would absolutely avoid naming things like this unless there were no better options.
The test class would need to be updated as well
| return successful; | ||
| } | ||
|
|
||
| static bool TryParseWildCard(string versionString, [NotNullWhen(true)] ref DataSetVersionRecord? version) |
There was a problem hiding this comment.
- Should be private?
- Typo - 'Wildcard' not 'WildCard'
There was a problem hiding this comment.
Thank you, and apologies, you left the typo comment before but it's crept in again with my refactor
| public static bool TryParse(string versionString, out DataSetVersionRecord? version) | ||
| { | ||
| version = null; | ||
| if (versionString.Contains("*")) |
There was a problem hiding this comment.
Can use '*' char here to save allocating a string
| if (versionString.Contains("*")) | ||
| return TryParseWildCard(versionString, ref version); |
There was a problem hiding this comment.
Kinda nitpicky, but we're going against the grain for most of the project's code style by not having if statements with braces.
The main issue is that it highlights as warnings in Rider (green squiggles):
In general, we consider most of the default formatting in Rider as the project's style. If you're not against using different tooling, there'll be fewer issues if you switch to Rider instead of VS.
Regardless of tooling, to avoid potentially causing friction later on with people updating the code to match the expected style, would suggest adding in the line breaks.
Same with the successful case below.
There was a problem hiding this comment.
I'll try to incorporate this with my IDE
| if (parts.Length > 3) | ||
| return false; |
There was a problem hiding this comment.
Similar to earlier comment - could add braces.
Same below as well for next conditional
| public static async Task<Either<ActionResult, DataSetVersion>> FindByVersion( | ||
| this IQueryable<DataSetVersion> queryable, | ||
| Guid dataSetId, | ||
| string version, | ||
| CancellationToken cancellationToken = default) | ||
| { | ||
| if (!VersionUtils.TryParse(version, out var semVersion)) | ||
| { | ||
| if (!DataSetVersionRecord.TryParse(version, out var parsedVersion)) |
There was a problem hiding this comment.
Similar to other comments - would suggest keeping conditional braces in
There was a problem hiding this comment.
Thanks Nick. I'll make it a habit of doing this (+ see if my IDE can help me).
mmoalin
deleted the
EES-5593-Allow-wildcard-data-set-versions-in-public-API
branch
This PR (originally #5559) adds datasets wildcard versioning support to the following API endpoints:
For example:
dataSetVersion=2.*should return the latest minor version in the v2 seriesdataSetVersion=2.1should return v2.1dataSetVersion=2.0should return v2.0dataSetVersion=2should return v2.0Huge thanks to @leeoades for your support on this.