Add verify checks for incorrect title markers #28716
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
frenzibyte
requested changes
Jul 3, 2024
Comment on lines
+46
to
+67
| [Test] | ||
| public void TestTvSizeMarker() | ||
| { | ||
| beatmap.BeatmapInfo.Metadata.Title += " (TV Size)"; | ||
| beatmap.BeatmapInfo.Metadata.TitleUnicode += " (TV Size)"; | ||
|
|
||
| var issues = check.Run(getContext(beatmap)).ToList(); | ||
|
|
||
| Assert.That(issues, Has.Count.EqualTo(0)); | ||
| } | ||
|
|
||
| [Test] | ||
| public void TestMalformedTvSizeMarker() | ||
| { | ||
| beatmap.BeatmapInfo.Metadata.Title += " (tv size)"; | ||
| beatmap.BeatmapInfo.Metadata.TitleUnicode += " (tv size)"; | ||
|
|
||
| var issues = check.Run(getContext(beatmap)).ToList(); | ||
|
|
||
| Assert.That(issues, Has.Count.EqualTo(2)); | ||
| Assert.That(issues.Any(issue => issue.Template is CheckTitleMarkers.IssueTemplateIncorrectMarker)); | ||
| } |
There was a problem hiding this comment.
These tests are quite repetitive / useless, and they're not covering more important cases like different writings of the format (e.g. (TV Ver.) / (Game Size) which are apparently deemed incorrect).
There was a problem hiding this comment.
Tests being repetitive is never a big deal although these would take pretty well to parameterising.
There was a problem hiding this comment.
Yeah, I mentioned this because it was quite painful to add more useful tests than just the case-sensitivity ones.
|
I honestly don't know whether this check is worth spending any time on, but assuming it is, I tried refactoring this into a state that looks to be at least mergable in my eyes, and I got stuck in some Regex weirdness that is making me regret this. diffdiff: diff --git a/osu.Game.Tests/Editing/Checks/CheckTitleMarkersTest.cs b/osu.Game.Tests/Editing/Checks/CheckTitleMarkersTest.cs
index a8f86a6d45..441dcdce3e 100644
--- a/osu.Game.Tests/Editing/Checks/CheckTitleMarkersTest.cs
+++ b/osu.Game.Tests/Editing/Checks/CheckTitleMarkersTest.cs
@@ -1,6 +1,7 @@
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.
+using System;
using System.Linq;
using NUnit.Framework;
using osu.Game.Beatmaps;
@@ -39,192 +40,128 @@ public void Setup()
[Test]
public void TestNoTitleMarkers()
{
- var issues = check.Run(getContext(beatmap)).ToList();
- Assert.That(issues, Has.Count.EqualTo(0));
+ performTest(string.Empty, string.Empty);
}
[Test]
public void TestTvSizeMarker()
{
- beatmap.BeatmapInfo.Metadata.Title += " (TV Size)";
- beatmap.BeatmapInfo.Metadata.TitleUnicode += " (TV Size)";
-
- var issues = check.Run(getContext(beatmap)).ToList();
-
- Assert.That(issues, Has.Count.EqualTo(0));
- }
-
- [Test]
- public void TestMalformedTvSizeMarker()
- {
- beatmap.BeatmapInfo.Metadata.Title += " (tv size)";
- beatmap.BeatmapInfo.Metadata.TitleUnicode += " (tv size)";
-
- var issues = check.Run(getContext(beatmap)).ToList();
-
- Assert.That(issues, Has.Count.EqualTo(2));
- Assert.That(issues.Any(issue => issue.Template is CheckTitleMarkers.IssueTemplateIncorrectMarker));
+ performTest("(TV Size)", "(TV Size)");
+ performTest("(Tv size)", "(TV Size)");
+ performTest("[TV Size]", "(TV Size)");
+ performTest("(TV Ver.)", "(TV Size)");
+ performTest("(TV Ver)", "(TV Size)");
}
[Test]
public void TestGameVerMarker()
{
- beatmap.BeatmapInfo.Metadata.Title += " (Game Ver.)";
- beatmap.BeatmapInfo.Metadata.TitleUnicode += " (Game Ver.)";
-
- var issues = check.Run(getContext(beatmap)).ToList();
-
- Assert.That(issues, Has.Count.EqualTo(0));
- }
-
- [Test]
- public void TestMalformedGameVerMarker()
- {
- beatmap.BeatmapInfo.Metadata.Title += " (game ver.)";
- beatmap.BeatmapInfo.Metadata.TitleUnicode += " (game ver.)";
-
- var issues = check.Run(getContext(beatmap)).ToList();
-
- Assert.That(issues, Has.Count.EqualTo(2));
- Assert.That(issues.Any(issue => issue.Template is CheckTitleMarkers.IssueTemplateIncorrectMarker));
+ performTest("(Game Ver.)", "(Game Ver.)");
+ performTest("(Game ver.)", "(Game Ver.)");
+ performTest("[Game Ver.]", "(Game Ver.)");
+ performTest("(Game Size)", "(Game Ver.)");
+ performTest("(Game Ver)", "(Game Ver.)");
}
[Test]
public void TestShortVerMarker()
{
- beatmap.BeatmapInfo.Metadata.Title += " (Short Ver.)";
- beatmap.BeatmapInfo.Metadata.TitleUnicode += " (Short Ver.)";
-
- var issues = check.Run(getContext(beatmap)).ToList();
-
- Assert.That(issues, Has.Count.EqualTo(0));
- }
-
- [Test]
- public void TestMalformedShortVerMarker()
- {
- beatmap.BeatmapInfo.Metadata.Title += " (short ver.)";
- beatmap.BeatmapInfo.Metadata.TitleUnicode += " (short ver.)";
-
- var issues = check.Run(getContext(beatmap)).ToList();
-
- Assert.That(issues, Has.Count.EqualTo(2));
- Assert.That(issues.Any(issue => issue.Template is CheckTitleMarkers.IssueTemplateIncorrectMarker));
+ performTest("(Short Ver.)", "(Short Ver.)");
+ performTest("(Short ver.)", "(Short Ver.)");
+ performTest("[Short Ver.]", "(Short Ver.)");
+ performTest("(Short Size)", "(Short Ver.)");
+ performTest("(Short Ver)", "(Short Ver.)");
}
[Test]
public void TestCutVerMarker()
{
- beatmap.BeatmapInfo.Metadata.Title += " (Cut Ver.)";
- beatmap.BeatmapInfo.Metadata.TitleUnicode += " (Cut Ver.)";
-
- var issues = check.Run(getContext(beatmap)).ToList();
-
- Assert.That(issues, Has.Count.EqualTo(0));
- }
-
- [Test]
- public void TestMalformedCutVerMarker()
- {
- beatmap.BeatmapInfo.Metadata.Title += " (cut ver.)";
- beatmap.BeatmapInfo.Metadata.TitleUnicode += " (cut ver.)";
-
- var issues = check.Run(getContext(beatmap)).ToList();
-
- Assert.That(issues, Has.Count.EqualTo(2));
- Assert.That(issues.Any(issue => issue.Template is CheckTitleMarkers.IssueTemplateIncorrectMarker));
+ performTest("(Cut Ver.)", "(Cut Ver.)");
+ performTest("(Cut ver.)", "(Cut Ver.)");
+ performTest("[Cut Ver.]", "(Cut Ver.)");
+ performTest("(Cut Size)", "(Cut Ver.)");
+ performTest("(Cut Ver)", "(Cut Ver.)");
}
[Test]
public void TestSpedUpVerMarker()
{
- beatmap.BeatmapInfo.Metadata.Title += " (Sped Up Ver.)";
- beatmap.BeatmapInfo.Metadata.TitleUnicode += " (Sped Up Ver.)";
-
- var issues = check.Run(getContext(beatmap)).ToList();
-
- Assert.That(issues, Has.Count.EqualTo(0));
- }
-
- [Test]
- public void TestMalformedSpedUpVerMarker()
- {
- beatmap.BeatmapInfo.Metadata.Title += " (sped up ver.)";
- beatmap.BeatmapInfo.Metadata.TitleUnicode += " (sped up ver.)";
-
- var issues = check.Run(getContext(beatmap)).ToList();
-
- Assert.That(issues, Has.Count.EqualTo(2));
- Assert.That(issues.Any(issue => issue.Template is CheckTitleMarkers.IssueTemplateIncorrectMarker));
+ performTest("(Sped Up Ver.)", "(Sped Up Ver.)");
+ performTest("(Sped Up ver.)", "(Sped Up Ver.)");
+ performTest("[Sped Up Ver.]", "(Sped Up Ver.)");
+ performTest("(Speed Up Ver.)", "(Sped Up Ver.)");
+ performTest("(Sped Up Ver)", "(Sped Up Ver.)");
}
[Test]
public void TestNightcoreMixMarker()
{
- beatmap.BeatmapInfo.Metadata.Title += " (Nightcore Mix)";
- beatmap.BeatmapInfo.Metadata.TitleUnicode += " (Nightcore Mix)";
-
- var issues = check.Run(getContext(beatmap)).ToList();
-
- Assert.That(issues, Has.Count.EqualTo(0));
- }
-
- [Test]
- public void TestMalformedNightcoreMixMarker()
- {
- beatmap.BeatmapInfo.Metadata.Title += " (nightcore mix)";
- beatmap.BeatmapInfo.Metadata.TitleUnicode += " (nightcore mix)";
-
- var issues = check.Run(getContext(beatmap)).ToList();
-
- Assert.That(issues, Has.Count.EqualTo(2));
- Assert.That(issues.Any(issue => issue.Template is CheckTitleMarkers.IssueTemplateIncorrectMarker));
+ performTest("(Nightcore Mix)", "(Nightcore Mix)");
+ performTest("(Nightcore mix)", "(Nightcore Mix)");
+ performTest("[Nightcore Mix]", "(Nightcore Mix)");
+ performTest("(Nightcore Ver.)", "(Nightcore Mix)");
+ performTest("(Nightcore Ver)", "(Nightcore Mix)");
+ performTest("(Night Core Mix)", "(Nightcore Mix)");
+ performTest("(Night Core Ver.)", "(Nightcore Mix)");
}
[Test]
public void TestSpedUpCutVerMarker()
{
- beatmap.BeatmapInfo.Metadata.Title += " (Sped Up & Cut Ver.)";
- beatmap.BeatmapInfo.Metadata.TitleUnicode += " (Sped Up & Cut Ver.)";
-
- var issues = check.Run(getContext(beatmap)).ToList();
-
- Assert.That(issues, Has.Count.EqualTo(0));
+ performTest("(Sped Up & Cut Ver.)", "(Sped Up & Cut Ver.)");
+ performTest("(Sped up & cut ver.)", "(Sped Up & Cut Ver.)");
+ performTest("[Sped Up & Cut Ver.]", "(Sped Up & Cut Ver.)");
+ performTest("(Speed Up & Cut Ver.)", "(Sped Up & Cut Ver.)");
+ performTest("(Sped Up & Cut Size)", "(Sped Up & Cut Ver.)");
+ performTest("(Sped Up Ver. & Cut Ver.)", "(Sped Up & Cut Ver.)");
+ performTest("(Sped Up Ver & Cut Ver.)", "(Sped Up & Cut Ver.)");
+ performTest("(SpedUp & Cut Ver.)", "(Sped Up & Cut Ver.)");
+ performTest("(Sped Up & Cut Ver)", "(Sped Up & Cut Ver.)");
}
[Test]
- public void TestMalformedSpedUpCutVerMarker()
+ public void TestNightcoreCutVerMarker()
{
- beatmap.BeatmapInfo.Metadata.Title += " (sped up & cut ver.)";
- beatmap.BeatmapInfo.Metadata.TitleUnicode += " (sped up & cut ver.)";
-
- var issues = check.Run(getContext(beatmap)).ToList();
-
- Assert.That(issues, Has.Count.EqualTo(2));
- Assert.That(issues.Any(issue => issue.Template is CheckTitleMarkers.IssueTemplateIncorrectMarker));
+ performTest("(Nightcore & Cut Ver.)", "(Nightcore & Cut Ver.)");
+ performTest("(Nightcore & cut ver.)", "(Nightcore & Cut Ver.)");
+ performTest("[Nightcore & Cut Ver.]", "(Nightcore & Cut Ver.)");
+ performTest("(Night Core & Cut Ver.)", "(Nightcore & Cut Ver.)");
+ performTest("(Nightcore Ver. & Cut Ver.)", "(Nightcore & Cut Ver.)");
+ performTest("(Nightcore Mix & Cut Ver.)", "(Nightcore & Cut Ver.)");
+ performTest("(Nightcore & Cut Size)", "(Nightcore & Cut Ver.)");
+ performTest("(Nightcore & Cut Ver)", "(Nightcore & Cut Ver.)");
+ performTest("(Nightcore Ver & Cut Ver.)", "(Nightcore & Cut Ver.)");
}
- [Test]
- public void TestNightcoreCutVerMarker()
+ private void performTest(string marker, string expected)
{
- beatmap.BeatmapInfo.Metadata.Title += " (Nightcore & Cut Ver.)";
- beatmap.BeatmapInfo.Metadata.TitleUnicode += " (Nightcore & Cut Ver.)";
-
- var issues = check.Run(getContext(beatmap)).ToList();
-
- Assert.That(issues, Has.Count.EqualTo(0));
+ performTest(marker, expected, false);
+ performTest(marker, expected, true);
}
- [Test]
- public void TestMalformedNightcoreCutVerMarker()
+ private void performTest(string marker, string expected, bool hasRomanisedTitle)
{
- beatmap.BeatmapInfo.Metadata.Title += " (nightcore & cut ver.)";
- beatmap.BeatmapInfo.Metadata.TitleUnicode += " (nightcore & cut ver.)";
+ beatmap.BeatmapInfo.Metadata.Title = "Egao no Kanata " + marker;
+
+ if (hasRomanisedTitle)
+ beatmap.BeatmapInfo.Metadata.TitleUnicode = "エガオノカナタ " + marker;
+ else
+ beatmap.BeatmapInfo.Metadata.TitleUnicode = beatmap.BeatmapInfo.Metadata.Title;
var issues = check.Run(getContext(beatmap)).ToList();
- Assert.That(issues, Has.Count.EqualTo(2));
- Assert.That(issues.Any(issue => issue.Template is CheckTitleMarkers.IssueTemplateIncorrectMarker));
+ if (marker == expected)
+ CollectionAssert.IsEmpty(issues);
+ else
+ {
+ if (hasRomanisedTitle)
+ {
+ Console.WriteLine(string.Join('\n', issues));
+ Assert.That(issues, Has.Count.EqualTo(2));
+ Assert.That(issues[0].Arguments[1], Is.EqualTo(expected));
+ Assert.That(issues[0].Arguments[2], Is.EqualTo(marker));
+ }
+ }
}
private BeatmapVerifierContext getContext(IBeatmap beatmap)
@@ -232,4 +169,4 @@ private BeatmapVerifierContext getContext(IBeatmap beatmap)
return new BeatmapVerifierContext(beatmap, new TestWorkingBeatmap(beatmap));
}
}
-}
\ No newline at end of file
+}
diff --git a/osu.Game/Rulesets/Edit/Checks/CheckTitleMarkers.cs b/osu.Game/Rulesets/Edit/Checks/CheckTitleMarkers.cs
index 2471d175ae..6d928bf006 100644
--- a/osu.Game/Rulesets/Edit/Checks/CheckTitleMarkers.cs
+++ b/osu.Game/Rulesets/Edit/Checks/CheckTitleMarkers.cs
@@ -3,73 +3,80 @@
using System.Collections.Generic;
using System.Text.RegularExpressions;
+using JetBrains.Annotations;
+using osu.Game.Localisation;
using osu.Game.Rulesets.Edit.Checks.Components;
namespace osu.Game.Rulesets.Edit.Checks
{
public class CheckTitleMarkers : ICheck
{
- public CheckMetadata Metadata => new CheckMetadata(CheckCategory.Metadata, "Checks for incorrect formats of (TV Size) / (Game Ver.) / (Short Ver.) / (Cut Ver.) / (Sped Up Ver.) / etc in title.");
+ private static readonly IReadOnlyList<MarkerFormatCheck> marker_checks = new[]
+ {
+ new MarkerFormatCheck(@"(TV Size)", @"(.tv (size|ver).*)"),
+ new MarkerFormatCheck(@"(Game Ver.)", @"(.game (size|ver).*)"),
+ new MarkerFormatCheck(@"(Short Ver.)", @"(.short (size|ver).*)"),
+ new MarkerFormatCheck(@"(Cut Ver.)", @"(?<!&)(.cut (size|ver).*)"),
+ new MarkerFormatCheck(@"(Sped Up Ver.)", @"(?<!&)(.(sped|speed) ?up ver.*)(?!&)"),
+ new MarkerFormatCheck(@"(Nightcore Mix)", @"(?<!&)(.(nightcore|night core) (ver|mix).?)(?!&)"),
+ new MarkerFormatCheck(@"(Sped Up & Cut Ver.)", @"(.(sped|speed) ?up (ver.?)? ?& cut (size|ver).*)"),
+ new MarkerFormatCheck(@"(Nightcore & Cut Ver.)", @"(.(nightcore|night core) (ver.?|mix)? ?& cut (size|ver).*)"),
+ };
+
+ public CheckMetadata Metadata => new CheckMetadata(CheckCategory.Metadata, "Incorrect format of markers such as \"(TV Size)\", \"(Game Ver.)\", etc. in title");
public IEnumerable<IssueTemplate> PossibleTemplates => new IssueTemplate[]
{
new IssueTemplateIncorrectMarker(this),
};
- private IEnumerable<MarkerCheck> markerChecks = [
- new MarkerCheck("(TV Size)", @"(?i)(tv (size|ver))"),
- new MarkerCheck("(Game Ver.)", @"(?i)(game (size|ver))"),
- new MarkerCheck("(Short Ver.)", @"(?i)(short (size|ver))"),
- new MarkerCheck("(Cut Ver.)", @"(?i)(?<!& )(cut (size|ver))"),
- new MarkerCheck("(Sped Up Ver.)", @"(?i)(?<!& )(sped|speed) ?up ver"),
- new MarkerCheck("(Nightcore Mix)", @"(?i)(?<!& )(nightcore|night core) (ver|mix)"),
- new MarkerCheck("(Sped Up & Cut Ver.)", @"(?i)(sped|speed) ?up (ver)? ?& cut (size|ver)"),
- new MarkerCheck("(Nightcore & Cut Ver.)", @"(?i)(nightcore|night core) (ver|mix)? ?& cut (size|ver)"),
- ];
-
public IEnumerable<Issue> Run(BeatmapVerifierContext context)
{
- string romanisedTitle = context.Beatmap.Metadata.Title;
- string unicodeTitle = context.Beatmap.Metadata.TitleUnicode;
+ string title = context.Beatmap.Metadata.Title;
+ string titleUnicode = context.Beatmap.Metadata.TitleUnicode;
+ bool hasRomanisedTitle = titleUnicode != title;
- foreach (var check in markerChecks)
+ foreach (var check in marker_checks)
{
- bool hasRomanisedTitle = unicodeTitle != romanisedTitle;
-
- if (check.AnyRegex.IsMatch(romanisedTitle) && !check.ExactRegex.IsMatch(romanisedTitle))
+ if (hasRomanisedTitle)
{
- yield return new IssueTemplateIncorrectMarker(this).Create(hasRomanisedTitle ? "Romanised title" : "Title", check.CorrectMarkerFormat);
- }
+ var romanisedMatch = check.Regex.Match(title);
+ if (romanisedMatch.Length > 0 && romanisedMatch.Value != check.Expectation)
+ yield return new IssueTemplateIncorrectMarker(this).Create(check.Expectation, romanisedMatch.Value, true);
- if (hasRomanisedTitle && check.AnyRegex.IsMatch(unicodeTitle) && !check.ExactRegex.IsMatch(unicodeTitle))
+ var unicodeMatch = check.Regex.Match(titleUnicode);
+ if (unicodeMatch.Length > 0 && unicodeMatch.Value != check.Expectation)
+ yield return new IssueTemplateIncorrectMarker(this).Create(check.Expectation, unicodeMatch.Value, false);
+ }
+ else
{
- yield return new IssueTemplateIncorrectMarker(this).Create("Title", check.CorrectMarkerFormat);
+ var match = check.Regex.Match(title);
+ if (match.Length > 0 && match.Value != check.Expectation)
+ yield return new IssueTemplateIncorrectMarker(this).Create(check.Expectation, match.Value, false);
}
}
}
- private class MarkerCheck
+ public class IssueTemplateIncorrectMarker : IssueTemplate
{
- public string CorrectMarkerFormat;
- public Regex ExactRegex;
- public Regex AnyRegex;
-
- public MarkerCheck(string exact, string anyRegex)
+ public IssueTemplateIncorrectMarker(ICheck check)
+ : base(check, IssueType.Problem, "Expected marker in {0} to be in the format \"{1}\", but is \"{2}\"")
{
- CorrectMarkerFormat = exact;
- ExactRegex = new Regex(Regex.Escape(exact));
- AnyRegex = new Regex(anyRegex);
}
+
+ public Issue Create(string expected, string actual, bool romanised) => new Issue(this, romanised ? EditorSetupStrings.RomanisedTitle.ToString() : EditorSetupStrings.Title.ToString(), expected, actual);
}
- public class IssueTemplateIncorrectMarker : IssueTemplate
+ private class MarkerFormatCheck
{
- public IssueTemplateIncorrectMarker(ICheck check)
- : base(check, IssueType.Problem, "{0} field has a incorrect format of marker {1}")
+ public readonly Regex Regex;
+ public readonly string Expectation;
+
+ public MarkerFormatCheck(string expectation, [RegexPattern] string pattern)
{
+ Expectation = expectation;
+ Regex = new Regex(pattern, RegexOptions.IgnoreCase);
}
-
- public Issue Create(string titleField, string correctMarkerFormat) => new Issue(this, titleField, correctMarkerFormat);
}
}
-}
\ No newline at end of file
+}Failing cases:
I'll just bail out from this PR and leave it to someone else because I spent too much time here. |
The fact that checking the unicode title was gated behind a `hasRomanisedTitle` guard was breaking my brain.
|
Upon closer inspection I find the bulk of @frenzibyte's review invalid. This is completely fine, and requesting any changes is excessive. I've done minor touch-ups to the logic and resolved code quality and that's it. |
bdach
previously approved these changes
Jul 3, 2024
frenzibyte
reviewed
Jul 3, 2024
frenzibyte
reviewed
Jul 3, 2024
bdach
approved these changes
Jul 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses a check listed at #12091.
Adds checks for incorrect formats of tv size, cut ver, nightcore mix, and the others, based on the Mapset Verifier code.