Added a new event enum value, "Festival" #1199
Conversation
| @@ -18,8 +18,9 @@ | |||
| "Club": "Club", | |||
| "Concert": "Concert", | |||
| "Contest": "Contest", | |||
| "Convention": "Convention", | |||
| "Other": "Other", | |||
| "Festival": "Festival", | |||
There was a problem hiding this comment.
You need to update not only json files, but also corresponding resx files on the VocaDB/vocadb-loc repo. These json files are intended to be automatically generated by this script (as part of #900). It would be nice if we could do this automatically at compile time though.
There was a problem hiding this comment.
added, VocaDB/vocadb-loc#2
| /// <summary> | ||
| /// J-POP Cover Festival | ||
| /// </summary> | ||
| Festival = 1 << 7, |
There was a problem hiding this comment.
Can you please add a unit test for the change? You can use one of the tests in the Tests/Domain folder (depending on whether or not numbers/names can be changed) as a reference. Additionally, can you please enable nullable reference types by removing the #nullable disable directive, and use the file-scoped namespace for this file? You can do this in a separate commit.
| @@ -5,6 +5,7 @@ | |||
| "Club": "クラブ", | |||
| "Concert": "コンサート", | |||
| "Contest": "コンテスト", | |||
| "Festival": "祭り", | |||
There was a problem hiding this comment.
Which should we use, "祭" or "祭り"?
There was a problem hiding this comment.
ohh that's a typo, I wanted to write "祭"
| [DataRow(0, EventCategory.Unspecified)] | ||
| [DataRow(1, EventCategory.AlbumRelease)] | ||
| [DataRow(2, EventCategory.Anniversary)] | ||
| [DataRow(4, EventCategory.Club)] | ||
| [DataRow(8, EventCategory.Concert)] | ||
| [DataRow(16, EventCategory.Contest)] | ||
| [DataRow(32, EventCategory.Convention)] | ||
| [DataRow(64, EventCategory.Other)] | ||
| [DataRow(128, EventCategory.Festival)] | ||
| [TestMethod] | ||
| public void Value(int expected, EventCategory actual) | ||
| { | ||
| ((int) actual).Should().Be(expected); | ||
| } |
There was a problem hiding this comment.
Since the values of the EventCategory enum are saved as a string in DB, and not as a bit field, you should write tests against names instead of numbers (and can you please make sure that?). See PVServiceTests.cs.
There was a problem hiding this comment.
Forgot to ask: if they are stored as strings, there should be no problem with switching the order of values in the enum class, right? Currently, when selecting event type, "Festival" comes last, while I'd like it to come immediately after "Contest" (I think it makes more sense this way)
There was a problem hiding this comment.
Switching the order may work if and only if you are 100 percent sure that they are never stored as numbers. It would be disastrous if you accidentally changed it. I would suggest switching the order on the frontend instead, just like ArtistTypesDropdownKnockout, because it's more flexible. You could define a varaible like (you may need to convert the EventCategory enum to a string enum first though):
const eventCategoryOrders: Record<EventCategory, number> = {
[EventCategory.Unspecified]: 1,
[EventCategory.AlbumRelease]: 2,
// ...
}
|
Thanks for the PR! |
Closes VocaDB/community#155.