-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 verify checks for incorrect title markers #28716
Conversation
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests being repetitive is never a big deal although these would take pretty well to parameterising.
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.
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 <contact@ppy.sh>. 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. |
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.
sure, works as advertised
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.