-
Notifications
You must be signed in to change notification settings - Fork 63
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
[Proposal][Feature] Convert DMM's ContentID to standard ID #112
Conversation
This is a good idea. How I originally envisioned users using Javinizer would be using scrapers in conjunction with each other to grab the necessary metadata fields (i.e. dmm for description, r18 for id, etc.). Because Dmm didn't natively support ID, I didn't include it and assumed a user would just use a different scraper to grab the ID. But you bring up a good point of standardizing the ID format to match the rest of the scrapers.
Definitely option 1 like you already implemented. This is similar to how the R18 scraper is already working.
While I think it's feasible to convert from ContentId => Id, I don't believe the other way around is possible, at least not in the original format that dmm uses with things like Let's just:
|
65ce530
to
9e2d7e5
Compare
I have presently made the following changes:
Remark If there's any other preferred way to do this, please let me know. |
src/Javinizer/Public/Get-DmmData.ps1
Outdated
@@ -45,7 +48,7 @@ function Get-DmmData { | |||
$movieDataObject = [PSCustomObject]@{ | |||
Source = if ($Url -match '/en/') { 'dmm' } else { 'dmmja' } | |||
Url = $Url | |||
Id = Get-DmmId -WebRequest $webRequest | |||
Id = if ($IdPreference -eq "id") { Get-DmmId -WebRequest $webRequest } elseif ($IdPreference -eq "contentid") { Get-DmmContentId -WebRequest $webRequest } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a bit of thinking about having the Id convert to ContentId directly on the scrapers themselves.
I think it would be better to change it only on the aggregated data object, since if people were using Javinizer solely for the scraper functions, they wouldn't want the original scraper output to be changed depending on an Id preference.
My approach would be to consolidate the settings into a single setting scraper.option.idpreference
and set the Id
during the foreach loop that assigns all the aggregated data fields.
foreach ($field in $metadataFields) {
$metadataPriority = (Get-Variable -Name "$($field)Priority" -ValueOnly)
foreach ($priority in $metadataPriority) {
$sourceData = $Data | Where-Object { $_.Source -eq $priority }
if ($null -eq $aggregatedDataObject.$field) {
if ($field -eq 'AlternateTitle') {
$aggregatedDataObject.$field = $sourceData.Title
} elseif ($field -eq 'Id') {
if ($IdPreference -eq 'contentid') {
$aggregatedDataObject.$field = $sourceData.ContentId
} else {
$aggregatedDataObject.$field = $sourcedata.Id
}
} else {
$aggregatedDataObject.$field = $sourceData.$field
}
Write-JVLog -Write:$script:JVLogWrite -LogPath $script:JVLogPath -WriteLevel $script:JVLogWriteLevel -Level Debug -Message "[$($Data[0].Id)] [$($MyInvocation.MyCommand.Name)] [$field - $priority] Set to [$($sourceData.$field | ConvertTo-Json -Compress)]"
}
}
}
What do you think of this approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally like this approach.
To be honest, I wasn't quite pleased with what I have done. I prefer not to have the logic encoded in the model (in this case $movieDataObject
). I will include this in the changes. Thank you for the suggestion!
577088e
to
b5b9116
Compare
b5b9116
to
df942c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
The current
ID
is rather inconsistent with all other web providers. For instance, DMM uses a format ofID#####
where # represents some kind of numeric value while others like Javlibrary uses the standardID-###
. Additionally, there's also no way to customize which type of ID I would like to use.This exhibits a problem because I would prefer to have it use
ID-###
rather thanID####
, but there is no way to reflect this choice in the settings file.In this PR, I would like to propose to standardize the
ID
format toID-###
and keepID#####
as a separate ID, calledContentID
. This would also allow customization insort.format.file
and others to support both<ID>
and<CONTID>
.There are two ways to go about this problem.
$Data
to supportContentId
andId
. Possibly, include another field that keeps the original scraped id, calledScrapeId
Convert-JVString
to support<CONTID>
.Each of the above choices has its own repercussions:
Convert-JVString
to cover all use cases, especially when new aggregators are added. At the moment, I don't see a large deviation in ID patterns.Right now, I think either choice is fine, but I will leave the final decision to you.
What do you think?
Personally, I have gone with the first choice, but if need be, I can implement the second choice.
With that, I've only made changes to DMM as a prototype, but if this proposal is accepted, I will continue to make changes for the others depending on the choice.
Checklist:
Id
andContentId
for DMM.ImplementContentId
for all other aggregators