search

pedal-edu / pedal

A collection of tools to analyze student's Python source code
https://pedal-edu.github.io/pedal/
MIT License
29 stars 7 forks source link

Cait2 branch changes #13

Closed acbart closed 5 years ago

acbart commented 5 years ago

The following is a summary of all the changes that I've made in the new branch cait2, so far. Need to walk through them and make sure that they are all valid changes.

1) Top-level API state has been restructured. There's no longer a Cait object, since that didn't appear to be doing anything. Instead, state is stored through whatever Report gets passed into the main Cait functions (parse_program, find_match, find_matches, etc.), defaulting to the MAIN_REPORT. 2) I changed the way TIFA's data is loaded. If TIFA hasn't been run, attempting to use its functions will just cause an error. The argument is that the user should know to run TIFA themselves before CAIT, not expect CAIT to run TIFA. 3) Renamed EasyNode to CaitNode. 4) Removed CtMap in favor of built-in Dictionaries. This required changing a couple tests - it seemed they relied on the particular ordering of CtMap's keys and values, which seemed like it might be fragile to me. An AstMap now has a match_lineno property to support this better. 5) Added CaitNode.is_method(), get_data_state(), and get_data_type() 6) StretchyTreeMatcher now returns empty lists instead of False values - I don't think this has any negative effects, and allows you to remove if checks before iterating through the matches. 7) AstSymbol now has a __getattr__ that will call its .ast_node attribute to getattr from there. This allows you to use the result of AstMap lookups more easily. 8) Changed AstSymbol.__str__ to be __repr__, and made __str__ just return the id attribute. This allows you to .format the results more easily. 9) Added CaitNode.find_matches which farms out a call to a new StretchyTreeMatcher with find_matches. The idea is to simplify subexpression matching:

# Old
submatches = find_expr_sub_matches("_target_.append(___)", __expr__)
# New
submatches = __expr__.find_matches("_target_.append(___)")

Changes I'm still considering:

9) AstMap.__getitem__ could automatically return the first item from the symbol_table's list, rather than returning a list of all potential items. I observe that almost every single Mistakes function and unittest use the first element. If someone needed a later element, they could just use AstMap.symbol_table to access it directly. This would require touching almost every test, but it seems like it'd be a nice change. 10) Fixing all the camelCase attributes. That's not consistent with the rest of the API and is bad Python style, but it'd be a fairly big refactor. In the meantime, I tried to add under_score_case attributes in a few places that allow access either way. Documentation should favor the under_score_case.

lukesg08 commented 5 years ago

I like the changes that you made. for the AstMap.getitem I created a workaround for that so that you can access it as both list, or an AstSymbol, so it supports a more flexible syntax. So the two asserts below would both evaluate to true

set_source("fun = 0\nfun = 1")
parse_program()
my_match = find_match("_fun_ = ___")
self.assertTrue(my_match["_fun_"].id == "fun")
self.assertTrue(my_match["_fun_"][0].id == "fun")

This was implemented by creating AstSymbolList as a wrapper for the dictionaries.

acbart commented 5 years ago

Ah, that seems like a good idea. We'll need to run this stuff through Skulpt/BlockPy to make sure it all works out, but this seems like some good improvements to me. I still have a bunch of Sandbox, Source, Resolver, and Assertions stuff to wrap up, but I think those are almost ready. Hopefully I can get them done by our meeting tomorrow morning.

lukesg08 commented 5 years ago

We finished this a while ago I'm pretty sure.