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

Don't set a default argument as a mutable type #4034

Merged
merged 2 commits into from Feb 22, 2018
Merged

Conversation

CarlQLange
Copy link
Contributor

When you run this a second time, headers will be the value they were last time you ran it. That seems bad.

http://docs.python-guide.org/en/latest/writing/gotchas/

Fixes #

Proposed fixes:

Features:

  • includes tests covering changes
  • includes updated documentation
  • includes user-visible changes
  • includes API changes
  • includes bugfix for possible backport

Please [X] all the boxes above that apply

When you run this a second time, `headers` will be the value they were last time you ran it. That seems bad.

http://docs.python-guide.org/en/latest/writing/gotchas/
@wardi
Copy link
Contributor

wardi commented Feb 22, 2018

looks clearly correct to me. @CarlQLange fix the pep8 issues and I'll merge

@wardi wardi merged commit 3251b50 into ckan:master Feb 22, 2018
@CarlQLange
Copy link
Contributor Author

@wardi Fixed them, sorry about that :)

@CarlQLange
Copy link
Contributor Author

It looks to me like this issue is in some other places in the codebase - I don't have time at the moment to fix all of these unfortunately. Here's the list and the command I used.

ack --type=python ".*=[{}\[\]]+.*\):"
ckan/tests/legacy/__init__.py
100:    def create_package(self, data={}, **kwds):

ckan/tests/legacy/lib/test_resource_search.py
60:    def res_search(self, query='', fields={}, terms=[], options=search.QueryOptions()):

ckan/tests/legacy/lib/__init__.py
8:def check_search_results(terms, expected_count, expected_packages=[]):

ckan/tests/legacy/lib/test_i18n.py
14:    def handle_request(self, session_language=None, languages_header=[]):

ckan/tests/legacy/functional/api/model/test_package.py
35:    def get_groups_identifiers(self, test_groups, users=[]):

ckan/tests/legacy/functional/api/model/test_relationships.py
300:                                 expected_relationships=[]):

ckan/tests/legacy/functional/api/base.py
42:    def get(self, offset, status=[200]):
47:    def post(self, offset, data, status=[200,201], *args, **kwds):
55:    def app_delete(self, offset, status=[200,201], *args, **kwds):

ckan/logic/action/get.py
55:def _filter_activity_by_user(activity_list, users=[]):

ckan/model/user.py
295:    def user_ids_for_name_or_id(self, user_list=[]):

ckan/model/resource.py
139:    def get_all_without_views(cls, formats=[]):

ckan/model/resource_view.py
59:    def delete_all(cls, view_types=[]):

ckan/lib/search/query.py
158:    def run(self, query=None, terms=[], fields={}, facet_by=[], options=None, **kwargs):
207:    def run(self, fields={}, options=None, **kwargs):

ckan/lib/cli.py
2386:    def _search_datasets(self, page=1, view_types=[]):
2427:    def create_views(self, view_plugin_types=[]):
2488:    def clear_views(self, view_plugin_types=[]):

ckan/lib/create_test_data.py
35:    def create_gov_test_data(cls, extra_users=[]):
39:    def create_family_test_data(cls, extra_users=[]):
45:    def create_group_hierarchy_test_data(cls, extra_users=[]):
138:            extra_user_names=[], extra_group_names=[]):
529:                          user_names=[]):

ckan/views/api.py
470:api.add_url_rule(u'/<int(min=1, max={0}):ver>'.format(API_MAX_VERSION),
477:api.add_url_rule(u'/<int(min=3, max={0}):ver>/action/<logic_function>'.format(

ckanext/resourceproxy/plugin.py
14:def get_proxified_resource_url(data_dict, proxy_schemes=['http','https']):
67:                          package, proxy_schemes=['http','https']):

ckanext/datastore/tests/test_plugin.py
196:    def _datastore_search(self, context={}, data_dict={}, fields_types={}, query_dict={}):

@CarlQLange
Copy link
Contributor Author

(To be clear, this behaviour in python is a little bonkers to me and I only found out about it today. I'm sure we can all be forgiven for this :) )

@wardi
Copy link
Contributor

wardi commented Feb 22, 2018

We're just trying not to add new pep8 issues. One giant PR that fixes all these little things makes more work for us when we backport important fixes, so we're not looking for that.

@CarlQLange
Copy link
Contributor Author

I don't follow. This behaviour creates by default undefined and unpredictable behaviour based on when functions are called and what arguments they were previously called with. Replacing the default argument with None and then adding a check to set the argument to whatever we want it to actually be is the correct fix and has nothing whatever to do with pep8. I understand that you dislike people running automated tools to fix random pep8 long lines or whatever but this is an actual functional issue.

@wardi
Copy link
Contributor

wardi commented Feb 22, 2018

my mistake, I thought you were pointing out more pep8 issues.

@CarlQLange
Copy link
Contributor Author

No problem, I've seen some of the automated pep8 PRs before. Can't blame you for that :)

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.

None yet

2 participants