search

musiKk / plyj

A Java parser written in Python using PLY.
Other
150 stars 69 forks source link

Changed model's implementation #10

Closed pradyunsg closed 10 years ago

pradyunsg commented 10 years ago

The code in the model.py has been refactored. Now the classes are now compatible with python's AST module and model.py has compatibility (mostly) with PEP-8.

The interface is the same except for the use of type instead of _type for it is fine to have type as an attribute, and the str of the same was misleading..

The changes pass all tests, but you might want to make your test cases more elaborate, for something might have broken...

musiKk commented 10 years ago

Ah well. I knew about the issue of single evaluation of default values but still fell into that trap.

Regarding the change with the _field variable: I personally don't like it that much but if Python does it, I'm all for it. Consistency FTW.

There seem to be some weird things going on with some of the names. Examples are "Conrepructor" (l.123), "abrepract" (l. 167)... I don't know if there is more (looks like s/st/rep/ or something).

pradyunsg commented 10 years ago

Sorry, it's a mistake, I have actually used a dirty regex to extract the info from your inits, so there is a fair chance of there being more of these. Well, you have to write unit tests for that... That's another issue, lack of unit tests. I'll make the change after some time as I am away from my laptop for a few day starting today. I'll fix it then, I'll write some unit tests for the same if I can.

musiKk commented 10 years ago

Merged. Thanks! :beer:

pradyunsg commented 10 years ago

@musiKk Could you tell me why we have those accept methods in the AST nodes?

musiKk commented 10 years ago

Uhm... it's an application of the visitor pattern commonly used to walk ASTs.

pradyunsg commented 10 years ago

So, could we do without them? We could implement (I already have) something like Python's ast.NodeVisitor.. I'll make yet another pull request for the same soon. We could discuss about it there if needed...

musiKk commented 10 years ago

Before you put in too much effort: I used this approach deliberately. It's more in line with how it's done in JDT. Putting accept methods into the nodes allows modifications to the control flow. Currently, if an accept method returns false, the contents of the corresponding source element are not visited. There sure are multiple ways to approach this pattern but I don't see any reason for changing the current design. It's not that the model changes frequently. The currently pending change is Java 8 and Java 9... well, I'm not sure we'll still be alive by then...