daviis / PyDucker

A static ducking tool for python 3 source code
2 stars 2 forks source link

Ifvisit2 #51

Closed Smokebird closed 9 years ago

Smokebird commented 9 years ago

Created new branch for visit_If because If also go uses visit_compare and visit_BoolOp and the master have these visits. Visit_if is mostly implemented with a few cases that have to be added.

daviis commented 9 years ago

1) In visit_while why did you comment out the visit of the body and visit of the orelse? 2) Isn't visit_pass supposed to be visit_Pass? All of the other ones are capital. 3) I dont think it is important to special case the list and dict in visit_Compare because we have to be able to do inheritance which will allow everything to be an object. 4) In visit_If there is a variable orelse which isn't defined. I fixed it so now it is.

In the future please keep the list of visits in alphabetical order (except for the visit_ClassDef and visit_FunDef), it makes things easier to find.

Smokebird commented 9 years ago
  1. In visit_while visit body and visit of the orelse i commented out since I made a pull from master so my branch they commented out in branch.
  2. visit_pass is supposed to be visit_Pass I will push a fix for this
  3. I will have rethink what visit_In should to check if an obj is in a list or dict and other operations like it. pus some of the visits in alphabetical order except for visit_ClassDef and visit_FunDef
Smokebird commented 9 years ago

for 1 i meant to say they were commented out in master branch were i made my pull so in my branch they were commented out.

daviis commented 9 years ago

I already pushed all of those changes into the branch. Fetch the head and check to see if my changes look right to you.

Isaac

On Thu, Dec 11, 2014 at 11:57 AM, Smokebird notifications@github.com wrote:

  1. In visit_while visit body and visit of the orelse i commented out since I made a pull from master so my branch they commented out in branch.
  2. visit_pass is supposed to be visit_Pass I will push a fix for this
  3. I will have rethink what visit_In should to check if an obj is in a list or dict and other operations like it. pus some of the visits in alphabetical order except for visit_ClassDef and visit_FunDef

— Reply to this email directly or view it on GitHub https://github.com/daviis/PyDucker/pull/51#issuecomment-66660023.

Smokebird commented 9 years ago

All the changes look right to me I only changed visit_Gt and visit_GtE so that they will be in alphabetical order. The Changes will cause an error if we check if obj is in a list or something.

daviis commented 9 years ago

What do you mean by the second sentence. Can you point me to a line number?

On Thu, Dec 11, 2014 at 12:20 PM, Smokebird notifications@github.com wrote:

All the changes look right to me I only changed visit_Gt and visit_GtE so that they will be in alphabetical order. The Changes will cause an error if we check if obj is in a list or something.

— Reply to this email directly or view it on GitHub https://github.com/daviis/PyDucker/pull/51#issuecomment-66663597.

Smokebird commented 9 years ago

The error accrues at line 175. because class str does not have method contains reach the operation In returns. So we have we have redo visit_In.

daviis commented 9 years ago

In reality str does have a contains but we just dont have it implemented in the subset that is our namespace yet.

On Thu, Dec 11, 2014 at 12:37 PM, Smokebird notifications@github.com wrote:

The error accrues at line 175. because class str does not have method contains reach the operation In returns. So we have we have redo visit_In.

— Reply to this email directly or view it on GitHub https://github.com/daviis/PyDucker/pull/51#issuecomment-66665906.

Smokebird commented 9 years ago

the error is that the str contains\ method does not take a dict. Also int does not have a contains\ method might be easier implement at latter date

daviis commented 9 years ago

The goal of the project is to see if some one writes this:

a = {1:'a', 2:'b'}
if a in 'some string':
    print(a)

Our program will say that class string, method contains, does takes a dictionary.

And if some one writes

if 1 in 123:
    print("got it") 

Our program will say that class int, does not have a method contains

The way that things are implemented now is the right way to do things.

Smokebird commented 9 years ago

It also gives an error for this

x = 'a'
dict = {'a' : 1, 'b' : 2}
if x in dict:
    print(a)

also gives class string, method contains does not take dict so I think were doing x.__contains__(dict) but we should be doing dict.__contains__(x)

daviis commented 9 years ago

I think this means that we will need to switch on "contains" because that is what is syntactically different instead of switching based on a type of "list" or "dict"

On Thu, Dec 11, 2014 at 1:44 PM, Smokebird notifications@github.com wrote:

It also gives an error for this

x = 'a' dict = {'a' : 1, 'b' : 2} if x in dict: print(a)

also gives class string, method contains does not take dict so I think were doing x.contains(dict) but we should be doingdict. contains(x)

— Reply to this email directly or view it on GitHub https://github.com/daviis/PyDucker/pull/51#issuecomment-66676183.

daviis commented 9 years ago

This is all implemented. Emily look at my changes and see if that aggress with what you had in mind. Jake can you use a fresh set of eyes on this to make sure the changes make sense.

JakeAlbee commented 9 years ago

This looks right to me and the changes make sense. I don't see why it's having issues merging though

Smokebird commented 9 years ago

The changes make sense to me. The merge issues do not make much sense.