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

Centralise supported file extensions to one helper class #30915

Merged
merged 3 commits into from
Nov 29, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Nov 29, 2024

Comment on lines +12 to +17
public static readonly string[] ALL_EXTENSIONS =
[
..VIDEO_EXTENSIONS,
..AUDIO_EXTENSIONS,
..IMAGE_EXTENSIONS
];
Copy link
Collaborator Author

@bdach bdach Nov 29, 2024

Choose a reason for hiding this comment

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

Primer for what this is: https://learn.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-12#collection-expressions

I find this to be a compelling usage of the syntax because any other way to write it in C# is veeeeeeeeeery clunky, but I will go the clunky route if usage of this syntax is considered not wanted.

Copy link
Member

Choose a reason for hiding this comment

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

I personally think it's fine as it shortens code while not looking extremely odd.

Copy link
Member

Choose a reason for hiding this comment

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

I think I'm okay with this one.

{
Caption = GameplaySettingsStrings.BackgroundHeader,
PlaceholderText = EditorSetupStrings.ClickToSelectBackground,
},
audioTrackChooser = new FormFileSelector(".mp3", ".ogg")
// `SupportedExtensions.AUDIO_EXTENSIONS` not used here specifically because it includes `.wav` for samples, which is strictly disallowed by ranking criteria
Copy link
Member

Choose a reason for hiding this comment

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

Dunno, I think it's probably fine to leave it on the user to get this right and we could just allow it (I think stable would, but no one has done a silly here).

Copy link
Collaborator Author

@bdach bdach Nov 29, 2024

Choose a reason for hiding this comment

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

I initially thought the same but then reverted it after checking RC and seeing it explicitly spelled out there. I think we should strive to make it literally impossible for users to breach RC via in-game means when it comes to such matters, but am not strongly against using the helper.

@peppy peppy merged commit 58efed4 into ppy:master Nov 29, 2024
10 checks passed
@bdach bdach deleted the extension-checks branch November 29, 2024 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants