Add GetPublishedAtAsync to BaseProjectManager #334
Conversation
…rns the DateTime of a given purl (with a specified version) for when it was published. Or returns null if it couldn't be found.
…the UploadTime property, so I added that functionality as well.
|
|
||
| /// <summary> | ||
| /// Gets the <see cref="DateTime"/> a package version was published at. | ||
| /// </summary> | ||
| /// <param name="purl">Package URL specifying the package. Version is mandatory.</param> | ||
| /// <param name="useCache">If the cache should be used when looking for the published time.</param> | ||
| /// <returns>The <see cref="DateTime"/> when this version was published, or null if not found.</returns> | ||
| public virtual Task<DateTime?> GetPublishedAtAsync(PackageURL purl, bool useCache = true) | ||
| { | ||
| throw new NotImplementedException("BaseProjectManager does not implement GetPublishedAtAsync."); | ||
| } |
There was a problem hiding this comment.
Nit: I wonder whether it would make sense to hold off on adding new methods like this to BaseProjectManager until there is 'critical mass' per se of implementations for them for the specific package managers.
Advertising a method like this for all package managers when it only actually works for a small subset is not a great developer experience to have by default.
There was a problem hiding this comment.
I get what you're saying. As far as I see it then we have two options. We have this one. And the other one is the consumer has to cast the BaseProjectManager to one of the 3 that implement the method then they can call it. Personally I would rather do it this way, but what do you think?
There was a problem hiding this comment.
CFS is the only consumer of this method today right? And if I recall correctly, we are planning to instantiate the correct type of project manager directly anyway?
If that's true, then BaseProjectManager only really serves callers who want to work with more than one type of package at a time / who do not know which type of project manager instance they are getting. If we don't really have any consumers of this data yet from that perspective, I wonder if that gives us time to hold off on adding it to BaseProjectManager until that use case either materializes, or when we finish implementing this for many/most package managers.
There was a problem hiding this comment.
Yeah that's a good point. I will have it in just the ones we need it in.
There was a problem hiding this comment.
Is the published time available in the metadata from the metadata method? The default behavior could be to call the metadata method and then return the time.
Agree though that it should ideally be implemented in some manner on most of the managers to belong in the base.
There was a problem hiding this comment.
Yes, and I confirmed it in the unit tests.
There was a problem hiding this comment.
It seems to me the upload time value in the metadata object should already be a DateTime.
There was a problem hiding this comment.
I could change it to parse it elsewhere that makes sense. I'll go do it real quick.
There was a problem hiding this comment.
What about having this in the base, once the metadata is returned as an object?
public async DateTime GetPublishTimeAsync(PackagURL Purl) => await GetMetaDataAsync(Purl)?.PublishTimeThis requires having a metadata object be returned
public class Metadata
{
public DateTime PublishTime {get; set;}
public string Raw { get; set }
}There was a problem hiding this comment.
Yeah that can be something we look into when we do the larger rework to Metadata.
There was a problem hiding this comment.
Per comments hold off on the base project manager implementation unless there is good coverage. But keep the methods such that it can be added later to the base.
I think it would make sense to change what is returned from base managers metadata call to be a parsed metadata object rather than the raw string - though the string itself could also be included as a field in the metadata object.
…hat implement it.
…ersion is always the version on the root object we get when calling GetMetadataAsync because it isn't called on a version (similar to NPM) This way we can get the latest version as a fallback for GetPackageMetadata without having to parse all the versions and sort through them. This also allowed me to re-enable the ignored tests for the PyPIProjectManager.
+1 to this (though it sounds too big for this PR of course). When callers are writing code against the |
…ssary work to get the uploadTime, and no longer uses the entire GetPackageMetadataAsync method to just take the one value.
I've been working on something very close to this, meeting with @bhuwan-agarwal tomorrow to discuss it further. |
…GetPackageMetadata call.
This allows us to not have to do the parsing in GetPublishedAtAsync.
It returns the DateTime of a given purl (with a specified version) for when it was published. Or returns null if it couldn't be found.
For CFS we are using this data, so having a direct method to get it vs calling GetPackageMetadata and only keeping the time would help. If you disagree no worries and we can just do that, this was just a fairly easy thing to whip up.
Also,
NPMProjectManagerwasn't settingUploadTimein theGetPackageMetadatacall.