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

Update Rarity Filters & Refactoring #2962

Merged
merged 3 commits into from
Dec 18, 2017
Merged

Update Rarity Filters & Refactoring #2962

merged 3 commits into from
Dec 18, 2017

Conversation

ZeldaZach
Copy link
Member

@ZeldaZach ZeldaZach commented Dec 16, 2017

Fixes #2952

This will implement rarity filter updates.

  • COMMON: c, common
  • UNCOMMON: u, uncommon
  • RARE: r, rare
  • MYTHIC RARE: m, mr, mythic rare

I also refactored the file I was working with as CLion said it was bad... Fixed up the colorless stuff as well so it's more in line with what the filter code written does.

I'll be fixing up contribution docs at some point this month.

@tooomm
Copy link
Member

tooomm commented Dec 17, 2017

This seems to consider all available rarities for a card.
So if there are different rarities for the same card in different sets it's found by both. 👍

tooomm
tooomm previously requested changes Dec 17, 2017
Copy link
Member

@tooomm tooomm left a comment

Choose a reason for hiding this comment

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

MYTHIC RARE: m, mr, mythic rare

One might search for mythic only too!
That returns no results at the moment.

}

template <class T>
FilterTreeBranch<T>::~FilterTreeBranch()
{
while (!childNodes.isEmpty())
{
Copy link
Member

Choose a reason for hiding this comment

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

Can you adjust the style guide and remove the instruction to skip braces for 1-liner blocks?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll get around to updating all the files this week

Choose a reason for hiding this comment

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

It seems every new dev skips braces on 1-liners until they discover it leads to errors later. Glad to hear that's not the desired style.

Copy link
Member

Choose a reason for hiding this comment

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

Went that way originally because it's very nice in scala. Of course scala is expression-oriented so a typical 'missing brace' code error will pretty much always make compiling fail instead of silently being weird. Doesn't make sense for C++

@ZeldaZach
Copy link
Member Author

Fixed @tooomm

@Cockatrice Cockatrice deleted a comment Dec 17, 2017
Copy link
Member

@tooomm tooomm left a comment

Choose a reason for hiding this comment

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

Thanks for adding that in!

@ZeldaZach ZeldaZach merged commit 2abfd3b into Cockatrice:master Dec 18, 2017
@ZeldaZach ZeldaZach deleted the fix_2952 branch December 18, 2017 00:43
@Lachee Lachee mentioned this pull request Dec 18, 2017
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.

rarity filter improvements
4 participants