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

Allow searching replays by is-MM and rating type. #2787

Merged
merged 4 commits into from
Apr 6, 2024

Conversation

Aquanim
Copy link
Contributor

@Aquanim Aquanim commented Jan 26, 2021

Also displays (primary) rating type on each replay page to non-admins.

@Aquanim Aquanim changed the title [WIP] Allow searching replays by is-MM and rating type. Allow searching replays by is-MM and rating type. Jan 27, 2021
Copy link
Member

@maackey maackey left a comment

Choose a reason for hiding this comment

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

Just a few minor suggestions after glancing it over.

@Aquanim
Copy link
Contributor Author

Aquanim commented Jan 30, 2021

minor - I'd group up "None" and "Any" just in case additional options are added in the future, it'll be less fragmented

Good point.

Shouldn't Model.GetRatingCategory() return "None" if Model.IsRatedMatch() is false? That would greatly simplify the view, which IMO shouldn't have lots of logic embedded -- it gets messy and hard to style.

That seems like a reasonable take to me, but I'd prefer to tackle it in a separate PR at a later date.

The conditional is redundant around the switch statement.

It is, but all the other filters have a similar conditional around them to check whether filtering on that category is active so I think this makes the file as a whole easier to read, even if it's a little weird in the context of that specific switch statement.

Has flag wont have translation to SQL so this wont work. It would crash unless you dod ToList before. And if you do you are copying battles to memory

dotnet/efcore#2852 suggests that HasFlag does have a translation as of 2016. Certainly it did not throw an error like b.IsRatedMatch() did when I used it in the same place, and I would expect that to have worked if I had mistakenly added a ToList earlier. Can you confirm that this is a problem, and if so what I'm missing?

(In particular, if zkinfra has to be able to work using an old version of LINQ then that would be important to know going forward.)

@Licho1 Licho1 enabled auto-merge (squash) April 6, 2024 10:17
@Licho1 Licho1 merged commit 2714e86 into ZeroK-RTS:master Apr 6, 2024
1 check passed
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.

3 participants