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

Fix/foldit leaderboard #394

Merged
merged 1 commit into from
Jul 19, 2013
Merged

Fix/foldit leaderboard #394

merged 1 commit into from
Jul 19, 2013

Conversation

adampalay
Copy link
Contributor

sorts the foldit top_n list by course

@jkarni
@johnhess

@jkarni
Copy link

jkarni commented Jul 15, 2013

Don't merge until we make the current course be passed by default to get_tops_n, and add an attribute that allows more courses/runs to be added to "course_list"

@staticmethod
def get_tops_n(n, puzzles=['994559']):
def get_tops_n(n, puzzles=['994559'], course_list=[]):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is dangerous, generally. I don't think you have a problem in this function as it stands, but using mutable values as default arguments in python often leads to memory leaks. Better practice, generally, is to use None, and then at the top of the function, say

if course_list is None:
    course_list = []

Copy link
Contributor

Choose a reason for hiding this comment

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

And, in fact, since the only thing you do w/ course list is check for [] and do one query, or do a different query, you can just use None as your signal value instead.

@jkarni
Copy link

jkarni commented Jul 19, 2013

👍

1 similar comment
@cpennington
Copy link
Contributor

👍

adampalay added a commit that referenced this pull request Jul 19, 2013
@adampalay adampalay merged commit 73e6e6f into master Jul 19, 2013
@adampalay adampalay deleted the fix/foldit-leaderboard branch July 19, 2013 19:09
chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014
chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014
e-kolpakov referenced this pull request in open-craft/edx-platform Mar 9, 2015
mattdrayer/fix_broken_tests: Updated stale dates throughout test suite
diegomillan pushed a commit to eduNEXT/edx-platform that referenced this pull request Sep 14, 2016
* dylanrhodes/cme_unpaid_users:
  Include unpaid registrations in user data table
xavierchan added a commit to xavierchan/edx-platform-1 that referenced this pull request May 20, 2019
Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022
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