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

Add [module:PublicationsByPlatform] #1952

Merged
merged 1 commit into from
Sep 28, 2024

Conversation

YoshiRulz
Copy link
Collaborator

resolves #1910

@adelikat
Copy link
Collaborator

@YoshiRulz : how is this PR coming along? the code looks good

@YoshiRulz
Copy link
Collaborator Author

YoshiRulz commented Aug 25, 2024

It's finished (helps if I commit before pushing). The problem is #1910 (comment)

[...the output] differs from the manual table in that it doesn't group any platforms together (list taken from IGameSystemService).
edit: ALSO I didn't implement feos' suggestion of checking for and omitting empty lists, so it differs from the manual table in that way too.

@YoshiRulz YoshiRulz force-pushed the pubs-by-platform-module branch from fb2b6be to 28156d6 Compare August 25, 2024 16:10
@adelikat
Copy link
Collaborator

Yeah that grouping is not any concept we have in our database, and we would lose it if we used the module instead. How important is it @vadosnaprimer ?

@YoshiRulz YoshiRulz force-pushed the pubs-by-platform-module branch from 28156d6 to 182f89d Compare August 25, 2024 16:11
@vadosnaprimer
Copy link
Collaborator

If grouping functionality is added to the module it'd be an overkill?

[module:PublicationsByPlatform|GB,GBC,SGB|NES,FDS|DS,DSi]

@vadosnaprimer
Copy link
Collaborator

vadosnaprimer commented Sep 22, 2024

Turns out we already have modules that have grouping. Example

[module:displaymovie|flag=verified|systemCode=gb,gbc,gba|obs]

I believe the same should be done here.

[module:PublicationsByPlatform|Groups=GB-GBC-SGB,NES-FDS,DS-DSi]

@YoshiRulz YoshiRulz force-pushed the pubs-by-platform-module branch from 182f89d to d64def6 Compare September 22, 2024 14:11
@YoshiRulz
Copy link
Collaborator Author

You can test with

[module:PublicationsByPlatform|groupings=A2600-A7800,Jaguar-JaguarCD,GB-SGB-GBC,DOS-DOOM-Windows-Linux,MSX-SVI3x8,NES-FDS,N64-N64DD,DS-DSi,Genesis-32X-SegaCD,SMS-GG-SG1000,SNES-BSX,PCE-PCECD-SGX]

@vadosnaprimer
Copy link
Collaborator

Grouping works!!!

Is it a big DB hit if only non-empty lists are shown?

@YoshiRulz
Copy link
Collaborator Author

YoshiRulz commented Sep 23, 2024

IDK ask adelikat

edit:

it woudl be a query per system, plus caching, or a complex query
either way, I'd recommend avoiding it for now

@YoshiRulz YoshiRulz force-pushed the pubs-by-platform-module branch from 9a369ce to 2ae437d Compare September 23, 2024 19:01
@YoshiRulz YoshiRulz marked this pull request as ready for review September 23, 2024 19:02
{
var extant = (await platforms.GetAll()).ToList();
List<IReadOnlyList<SystemsResponse>> rows = [];
void ProcessGroup(string groupStr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this local function necessary here? It seems to be called only once, and it makes the entire method more difficult to parse and understand, because I have to keep going up and down to see what things are affected where.

Copy link
Collaborator Author

@YoshiRulz YoshiRulz Sep 24, 2024

Choose a reason for hiding this comment

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

It's that or goto, since C# doesn't have multi-break.

edit: I considered having the method return a List<...>? and then conditionally appending to rows in the outer loop; do you think that would be better?

@adelikat adelikat merged commit dbc6836 into TASVideos:main Sep 28, 2024
1 check passed
@YoshiRulz YoshiRulz deleted the pubs-by-platform-module branch September 28, 2024 20:38
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.

Automate the per-platform movie table
4 participants