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

var(['x','y']) should work but doesn't #12247

Closed
williamstein opened this issue Jan 2, 2012 · 13 comments
Closed

var(['x','y']) should work but doesn't #12247

williamstein opened this issue Jan 2, 2012 · 13 comments

Comments

@williamstein
Copy link
Contributor

We were very surprised that var(['x','y']) doesn't work at the JMM QA session.

Apply attachment: trac_12247_var_construction-fix_typo.patch

CC: @vbraun

Component: symbolics

Keywords: var

Author: Volker Braun

Reviewer: Burcin Erocal

Merged: sage-5.0.beta0

Issue created by migration from https://trac.sagemath.org/ticket/12247

@burcin
Copy link

burcin commented Jan 2, 2012

comment:1

This probably broke while fixing #7496.

@vbraun
Copy link
Member

vbraun commented Jan 2, 2012

Author: Volker Braun

@vbraun
Copy link
Member

vbraun commented Jan 2, 2012

comment:2

Well I was not aware that its supposed to work and the docstring doesn't mention it. The syntax

var('x, y')

is more compact imho. But then, why not allow a list/tuple of strings. Attached patch implements this.

@nbruin
Copy link
Contributor

nbruin commented Jan 2, 2012

comment:3

The pythonic thing would be to realize that the important part of the list here is that it is an iterable and hence accepting an iterable might be the right thing to do. But since strings are iterable too, this doesn't work.

So either lists or strings would need to be special cased. This breaks subclassing (if someone writes a class that is supposed to mimic strings or lists, how do you tell which is which? isinstance doesn't work on builtins, and multiple inheritance would mean you would not necessarily be able to decide based on that anyway. Look in the mro which of list or str you meet first?)

Perhaps safer is to accept var('x','y') and hence also var(*['x','y']).

@vbraun
Copy link
Member

vbraun commented Jan 2, 2012

comment:4

You are not supposed to use var() from code, so I don't think accepting more general iterables is particularly desirable.

@vbraun
Copy link
Member

vbraun commented Jan 2, 2012

Updated patch

@vbraun
Copy link
Member

vbraun commented Jan 2, 2012

comment:5

Attachment: trac_12247_var_construction.patch.gz

The new patch makes var a variadic function, so var('x','y') works as well.

@burcin
Copy link

burcin commented Jan 3, 2012

fix typo in Volker's patch - apply only this one

@burcin
Copy link

burcin commented Jan 3, 2012

Changed keywords from none to var

@burcin

This comment has been minimized.

@burcin
Copy link

burcin commented Jan 3, 2012

comment:6

Attachment: trac_12247_var_construction-fix_typo.patch.gz

I attached a new patch attachment: trac_12247_var_construction-fix_typo.patch which fixes a typo tree -> three in Volker's patch.

Volker's changes look good to me. I am switching this to a positive review.

@burcin
Copy link

burcin commented Jan 3, 2012

Reviewer: Burcin Erocal

@jdemeyer
Copy link

Merged: sage-5.0.beta0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants