Skip to content

Conversation

@ThaPwned
Copy link
Contributor

No description provided.

@ThaPwned
Copy link
Contributor Author

Yes, thank you! 'Em copy and paste mistakes...

It works, they said... who'd have thought testing was a thing, huh
@Ayuto
Copy link
Member

Ayuto commented Jan 26, 2016

Thanks for the PR!

Ayuto added a commit that referenced this pull request Jan 26, 2016
Added parent_menu to PagedESCMenu and PagedRadioMenu
@Ayuto Ayuto merged commit f89ddb9 into Source-Python-Dev-Team:master Jan 26, 2016
@ThaPwned ThaPwned deleted the parent_menu branch January 26, 2016 21:07
@Mahi
Copy link
Contributor

Mahi commented Jan 29, 2016

Not like it really matters, but there are two of these lines:

if not page_index and self.parent_menu is not None:

Both of which should probably explicitly check for page_index == 0 instead of not page_index.

@Ayuto
Copy link
Member

Ayuto commented Jan 29, 2016

True, but if we are already nit picking, we should use a constant like FIRST_PAGE. :P

@Mahi
Copy link
Contributor

Mahi commented Jan 29, 2016

I believe you missed my point here. It's not just a semantic change, changing not page to page_index == 0 actually changes the behaviour of the interpreter. If someone were to do something wrong and accidentally managed to set the page_index to an empty list, the not page_index check would evaluate as True.

@satoon101
Copy link
Member

Well, playing devil's advocate, if we believe "we're all adults", then if someone decides to change page_index to anything other than an integer, they deserve to have issues.

@Ayuto
Copy link
Member

Ayuto commented Jan 29, 2016

I believe you missed my point here.

No, I got your point. That's why I agreed, but this is really just a minor detail.

Btw. you can't set the page index to something else than an integer. set_player_page() validates the input. But even if you set it to an empty list, you will run into errors when the menu is refreshed.

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.

4 participants